Skip to content

Commit

Permalink
Clarify the S < l malleability test
Browse files Browse the repository at this point in the history
  • Loading branch information
@CodesInChaos
CodesInChaos committed May 7, 2014
1 parent 1d19ecd commit 2c86134
Showing 1 changed file with 14 additions and 2 deletions.
16 changes: 14 additions & 2 deletions Chaos.NaCl.Tests/Ed25519Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,19 @@ private byte[] AddL(IEnumerable<byte> input)
return result;
}

private byte[] AddLToSignature(byte[] signature)
{
return signature.Take(32).Concat(AddL(signature.Skip(32))).ToArray();
}

// Ed25519 is malleable in the `S` part of the signature
// One can add (a multiple of) the order of the subgroup `l` to `S` without invalidating the signature
// I consider rejecting signatures with S >= l
// The implementation only checks if the 3 high bits are zero, which is equivalent to checking if S < 2^253
// since `l` is only slightly larger than 2^252 this means that you can add `l` to almost every signature
// *once* without violating this condition, adding it twice will exceed 2^253 causing the signature to be rejected
// This test serves to document the *is* behaviour, and doesn't define *should* behaviour
//
// I consider rejecting signatures with S >= l, but should probably talk to upstream and libsodium before that
[TestMethod]
public void MalleabilityAddL()
{
Expand All @@ -109,8 +119,10 @@ public void MalleabilityAddL()
Ed25519.KeyPairFromSeed(out pk, out sk, new byte[32]);
var signature = Ed25519.Sign(message, sk);
Assert.IsTrue(Ed25519.Verify(signature, message, pk));
var modifiedSignature = signature.Take(32).Concat(AddL(signature.Skip(32))).ToArray();
var modifiedSignature = AddLToSignature(signature);
Assert.IsTrue(Ed25519.Verify(modifiedSignature, message, pk));
var modifiedSignature2 = AddLToSignature(modifiedSignature);
Assert.IsFalse(Ed25519.Verify(modifiedSignature2, message, pk));
}

[TestMethod]
Expand Down

2 comments on commit 2c86134

@jedisct1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Just noticed that you were considering rejecting signatures with S>=l in your implementation.

Thinking about releasing the next version of libsodium with the check as well. We know of at least one valid use case, and I can't think of any downsides of being less liberal.

Are you still considering this?

@CodesInChaos
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jedisct1 I'm not sure what's the best way to proceed.

API wise I see three possible approaches:

  1. Modify the existing verification function
  2. Add a VerifyStrict verification function
  3. Add an IsStandard/IsCanonical helper function

I'm not a fan of variant 2. I prefer variant 3 if few Ed25519 implementations include the checks to avoid incompatiblities and variant 1 if we get broad agreement between implementers on which checks to perform.

I'm also not sure which properties to validate. S < l isn't the only way a signature can be non standard:

  1. S < l

    Security issues: Malleable
    Verification cost: Cheap. Just an inequality.

  2. R.y >= P where P is the order of the field

    Security issues: Malleable, but only if adding P doesn't overflow into the next bit. For Ed25519 this only affects 19 points, so without a malicious signer this doesn't happen. Malicious signers can already produce varying signatures by choosing a different nonce.
    Verification cost: Cheap. Just an inequality, and we need the same code for S < l, so the complexity is low a s well.

  3. R not on the curve but on the twist

    Security issues: none
    Verification cost: Cheap. Check the curve equation which costs a handful of field multiplications.

  4. Low order R

    Security issues: Signer doesn't commit to a message. See It's possible to forge messages that crypto_sign_open verifies if the public key is zero. jedisct1/libsodium#112 for a discussion.
    Verification cost: cheap (compare against a blacklist (a dozen or so entries), or multiply with 8 and check against even fewer values.

  5. R not in the subgroup generated by the base point B

    Security issues: none
    Verification cost: A naive implementation would be expensive. It's probably possible to merge it with the scalar multiplication, but that adds complexity and might not work at all with batch verification.

Verifiying 0-3 is cheap. 4 is expensive and has no security impact. So verifying 0-3 sounds like a decent plan.

(It's possible that properties where I didn't think of a security impact actually do have one.)

Please sign in to comment.