Why Idenity is not used during verifier calculation?

[JexSrs]

Why Identity is not used during verifier calculation? According to some posts like this one it is a big security flaw. Also RFC 5054 uses identity during hashing. Is there a reason why the user's identity (username/email) is skipped?

That was found here and here.

[bgrosse-midokura]

I think it is addressed in https://github.com/midonet/tssrp6a#recommendations

The client can choose to exclude the identity of its computations or not. If excluded, the id cannot be changed.

But, I think this might be wrong, exactly the opposite: if included, then the ID cannot be changed.

The implementation would need to override computeIdentityHash as you pointed out, I'd guess just concatenating the the identity + password.

The reason for why it's not done by default in the library? We matched the existing java "nimbus" implementation, so I guess there it is not included.

Not closing the issue, because I think now we should decide if the library should include identifier in default settings.

[JexSrs]

I understand, please keep an update here in case of any changes for this matter. Thank you.

Edit: In computeIdentityHash I can do as you said an concat it, what must change in here? Should I just add bigIntToArrayBuffer(_I) and bigIntToArrayBuffer(_s) at the start of the hash function?

[bgrosse-midokura]

Looking into that, meanwhile I can confirm that the reason we don't include identity is Nimbus: https://github.com/midonet/tssrp6a#notes

[JexSrs]

I understand what about this?

In computeIdentityHash I can do as you said an concat it, what must change in here? Should I just add bigIntToArrayBuffer(_I) and bigIntToArrayBuffer(_s) at the start of the hash function?

Should I change only the computeIdentityHash or the computeClientEvidence too?

[bgrosse-midokura]

so I just tested: both will work, independently or together, as long as both client and server match the routine(s).

[JexSrs]

Perfect, can we safely say that will be more secure if we add it to both of them?

[bgrosse-midokura]

    class IdentityRoutines extends SRPRoutines { ‣ 1 reference
      public async computeIdentityHash(I: string, P: string): Promise<ArrayBuffer> { ‣ 3 references
        return await this.hash(stringToArrayBuffer(I), stringToArrayBuffer(P));
      }
      public async computeClientEvidence( ‣ 3 references
        I: string,
        s: bigint,
        A: bigint,
        B: bigint,
        S: bigint,
      ): Promise<bigint> {
        return arrayBufferToBigInt(
          await this.hash(
            stringToArrayBuffer(I),
            bigIntToArrayBuffer(s),
            bigIntToArrayBuffer(A),
            bigIntToArrayBuffer(B),
            bigIntToArrayBuffer(S),
          ),
        );
      }
    }

[bgrosse-midokura]

Perfect, can we safely say that will be more secure if we add it to both of them?

I will open a PR to discuss this further, I guess defaulting to including the identity is a major breaking change.

[JexSrs]

I will wait for updates, you can close this issue if you see fit.

[bgrosse-midokura]

I just found this https://github.com/midonet/tssrp6a/blob/master/test/srp6a.test.ts#L14 :sweat_smile:

[bgrosse-midokura]

But I'm left wondering about computeClientEvidence...

[JexSrs]

I agree, I believe we cannot change it the same way using extends because it belongs in utils which is not a class. Anyway I solved it by forking the project and change it directly. Also change all the code from async to sync, async seems unnecessary in case where no actual async functions where called.

[bgrosse-midokura]

Async is needed when using async crypto APIs, like browser's WebCrypto IIRC.

I prefer to have one async implementation instead of one async and one sync.

[bgrosse-midokura]

I just tested, nimbus fails if computeClientEvidence includes the identity and salt - but I'm not 100% if that is truly related to computeIdentityHash.

[JexSrs]

I see, well I do not need browser implementation so I discarted WebCrypto from the code. Yes I just tested that as yell (computeClientEvidence) and no use (not sure why tho), but fortunately the typescript works just fine by including it to both functions.