Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Padding of g #12

Open
yallie opened this issue May 22, 2018 · 29 comments · May be fixed by #13
Open

Padding of g #12

yallie opened this issue May 22, 2018 · 29 comments · May be fixed by #13

Comments

@yallie
Copy link
Contributor

yallie commented May 22, 2018

Hello,

I'd like to thank for the excellent SRP-6a implementation!
Nice API, very clean code, so easy to follow 👍
I have a .NET backend, so I've converted the code to C#.

While porting the code I noticed that the library doesn't pad the value of g.
RFC5054 specifies that g should be left-padded with zeros to be the same length as N.

The leads to the miscalculated value of the k multiplier: k = H(N | PAD(g)).
As a result, the code doesn't pass the SRP test vectors (RFC5054, Appendix B).
Namely, the values of k, B and S don't match.

The library works just fine without the padding as long as the same code
is used on both client and server. But if the server code strictly follows the RFC,
the client won't be able to authenticate because of the different values of k.

Shouldn't it be fixed?
@yallie
Copy link
Contributor Author

yallie commented May 22, 2018

A couple of notes:

  1. Fixing the padding of g won't affect existing credentials: k is not used to derive the private key.
  2. RFC5054 uses sha1, so the test vectors are not directly applicable (unless params.js also use sha1).

@LinusU
Copy link
Owner

LinusU commented May 23, 2018 •
edited
Loading

Nice API, very clean code, so easy to follow

Thank you 🙌

RFC5054 specifies that g should be left-padded with zeros to be the same length as N.

Hmm, yeah this should probably be fixed. Although RFC5054 specifies specifically how to use SRP as an authentication strategy for the TLS protocol, it seems like most libraries are following it so that seems to be the de facto standard for SRP.

It also seems that A and B should be padded when calculating u.

I'm trying to look at a few other libraries to see what they are doing:

Libray k u M H(A, M, K) ping
RFC5054 padded padded
pysrp (rfc5054_enable) padded padded only g not
srp-client padded padded ? padded @symeapp
srp-rb padded padded padded padded @lamikae
go-srp not not not ? @opencoff
SRP.swift padded padded not not
srptools padded padded not not
srp-6a-demo (PHP) not not not not @RuslanZavacky
DragonSRP padded padded not not
srp4net padded padded not not
Eneter padded not not not not on Github

Feels quite clear that we should pad k and u.

I've pinged the authors of implementations that don't follow that here, feel free to chime into the discussion here!

It's going to be a pain to update this for me though, already have multiple clients in production that are distributed in different channels 😂

LinusU added a commit that referenced this issue May 23, 2018
@LinusU
💥 Pad values following RFC5054
44840c8 
Fixes #12

Migration guide:

While verifiers generated with an earlier version of this library will continue to work, the client and the server must either both be using the padding, or both not.

The recommended upgrade path is to have both versions running on your server, which gives you time to upgrade all the clients. This can be acomplished by depending on this library two times:

```js
{
  // ...
  "dependencies": {
    // ...
    "secure-remote-password": "^0.4.0",
    "secure-remote-password-pre-rfc5054": "LinusU/secure-remote-password#pre-rfc5054"
  }
}
```

You will then have to implement a second verison of your endpoint, which uses the newer version.

```js
const loginCtrl1 = require('./controller/login-v1')
const loginCtrl2 = require('./controller/login-v2')

app.post('/v1/login-sessions', loginCtrl1.initiate)
app.post('/v1/login-sessions/:sessionId/finalize', loginCtrl1.finalize)

app.post('/v2/login-sessions', loginCtrl2.initiate)
app.post('/v2/login-sessions/:sessionId/finalize', loginCtrl2.finalize)
```

The `login-v1` controller should import the library as such:

```js
require('secure-remote-password-pre-rfc5054/server')
```

The `login-v2` controller should import the library as usual:

```js
require('secure-remote-password/server')
```
LinusU added a commit that referenced this issue May 23, 2018
@LinusU
💥 Pad values following RFC5054
ec5636a 
Fixes #12

Migration guide:

While verifiers generated with an earlier version of this library will continue to work, the client and the server must either both be using the padding, or both not.

The recommended upgrade path is to have both versions running on your server, which gives you time to upgrade all the clients. This can be acomplished by depending on this library two times:

```js
{
  // ...
  "dependencies": {
    // ...
    "secure-remote-password": "^0.4.0",
    "secure-remote-password-pre-rfc5054": "LinusU/secure-remote-password#pre-rfc5054"
  }
}
```

You will then have to implement another version of your endpoint, which uses the newer version.

```js
const loginCtrl1 = require('./controller/login-v1')
const loginCtrl2 = require('./controller/login-v2')

app.post('/v1/login-sessions', loginCtrl1.initiate)
app.post('/v1/login-sessions/:sessionId/finalize', loginCtrl1.finalize)

app.post('/v2/login-sessions', loginCtrl2.initiate)
app.post('/v2/login-sessions/:sessionId/finalize', loginCtrl2.finalize)
```

The `login-v1` controller should import the library as such:

```js
require('secure-remote-password-pre-rfc5054/server')
```

The `login-v2` controller should import the library as usual:

```js
require('secure-remote-password/server')
```
LinusU added a commit that referenced this issue May 23, 2018
@LinusU
💥 Pad values following RFC5054
73e5f73 
Fixes #12

Migration guide:

While verifiers generated with an earlier version of this library will continue to work, the client and the server must either both be using the padding, or both not.

The recommended upgrade path is to have both versions running on your server, which gives you time to upgrade all the clients. This can be accomplished by depending on this library two times:

```js
{
  // ...
  "dependencies": {
    // ...
    "secure-remote-password": "^0.4.0",
    "secure-remote-password-pre-rfc5054": "LinusU/secure-remote-password#pre-rfc5054"
  }
}
```

You will then have to implement another version of your endpoint, which uses the newer version.

```js
const loginCtrl1 = require('./controller/login-v1')
const loginCtrl2 = require('./controller/login-v2')

app.post('/v1/login-sessions', loginCtrl1.initiate)
app.post('/v1/login-sessions/:sessionId/finalize', loginCtrl1.finalize)

app.post('/v2/login-sessions', loginCtrl2.initiate)
app.post('/v2/login-sessions/:sessionId/finalize', loginCtrl2.finalize)
```

The `login-v1` controller should import the library as such:

```js
require('secure-remote-password-pre-rfc5054/server')
```

The `login-v2` controller should import the library as usual:

```js
require('secure-remote-password/server')
```
@LinusU LinusU linked a pull request May 23, 2018 that will close this issue
Open
@yallie
Copy link
Contributor Author

yallie commented May 23, 2018

Thanks for the detailed analysis! 👍
My 2 cents:

Library k u M H(A, M, K)
srp4net padded padded not not
Eneter padded not not not

It also seems that A and B should be padded when calculating u.

Good catch, I missed that!
I only looked at numbers that didn't match the test vectors.

It's going to be a pain to update this for me though, already have multiple clients in production

That's the problem :)

@LinusU
Copy link
Owner

LinusU commented May 23, 2018

My 2 cents

Awesome, adding them to the list!

That's the problem :)

I wrote a migration guide in #13, it's a bit inconvenient, but not too bad ☺️

@LinusU
Copy link
Owner

LinusU commented May 23, 2018

So I tried getting pysrp to output all the values, and then plug them in here to see that everything works. Turns out that they are doing another padding which makes it incompatible:

https://github.com/cocagne/pysrp/blob/9d43e9a35e354a1ff1c6abdf12276922cadef97e/srp/_pysrp.py#L196-L205

Instead of doing

M = H(H(N) xor H(g), H(I), s, A, B, K)

they are doing:

M = H(H(N) xor H(PAD(g)), H(I), s, A, B, K)

🤔

@yallie any idea which one is correct?

@LinusU
Copy link
Owner

LinusU commented May 23, 2018 •
edited
Loading

Lib  
pysrp (rfc5054_enable) H(PAD(g))
SRP.swift H(g)
srptools H(g)
DragonSRP H(g)

Seems like maybe pysrp is the outlier here 🤔

ping @cocagne

@yallie
Copy link
Contributor Author

yallie commented May 23, 2018 •
edited
Loading

Looks like RFC5054 doesn't specify how M is calculated.
And the original SRP-RFC doesn't specify where padding is required.
Too bad 😕

I'd say that g should be padded anywhere its value is hashed.
If k uses padded g, then M should also use padded g.

UPD. Wikipedia SRP example doesn't use padding:

print("6. client sends proof of session key to server")
M_c = H(H(N) ^ H(g), H(I), s, A, B, K_c)

@LinusU
Copy link
Owner

LinusU commented May 23, 2018

Hmm, yeah potentially, although one other way to see it is that it was padded in the other case to make it as long as the other part of the concatenation...

🤔

I'm going to leave this open for a while to see if anyone of the pinged persons responds. Whatever we decide on, I could go and submit pull requests to all the linked repositories so that at least everyone uses the same.

also ping @Bouke, @idlesign, @slechta and @osorin

@opencoff
Copy link

opencoff commented May 23, 2018 via email

@yallie
Copy link
Contributor Author

yallie commented May 23, 2018 •
edited
Loading

Looks like Stanford reference SRP-6 implementation doesn't use padding in H(N) ^ H(g):

// srp6_client.c, line 94
static SRP_RESULT
srp6_client_params(SRP * srp, const unsigned char * modulus, int modlen,
		   const unsigned char * generator, int genlen,
		   const unsigned char * salt, int saltlen)
{
  int i;
  unsigned char buf1[SHA_DIGESTSIZE], buf2[SHA_DIGESTSIZE];
  SHA1_CTX ctxt;

  /* Fields set by SRP_set_params */

  /* Update hash state */
  SHA1Init(&ctxt);
  SHA1Update(&ctxt, modulus, modlen);
  SHA1Final(buf1, &ctxt);	/* buf1 = H(modulus) */

  SHA1Init(&ctxt);
  SHA1Update(&ctxt, generator, genlen); /* <------------------ g is not padded */
  SHA1Final(buf2, &ctxt);	/* buf2 = H(generator) */

  for(i = 0; i < sizeof(buf1); ++i)
    buf1[i] ^= buf2[i];		/* buf1 = H(modulus) xor H(generator) */

  /* hash: H(N) xor H(g) */
  SHA1Update(&CLIENT_CTXP(srp)->hash, buf1, sizeof(buf1));

  SHA1Init(&ctxt);
  SHA1Update(&ctxt, srp->username->data, srp->username->length);
  SHA1Final(buf1, &ctxt);	/* buf1 = H(user) */

  /* hash: (H(N) xor H(g)) | H(U) */
  SHA1Update(&CLIENT_CTXP(srp)->hash, buf1, sizeof(buf1));

  /* hash: (H(N) xor H(g)) | H(U) | s */
  SHA1Update(&CLIENT_CTXP(srp)->hash, salt, saltlen);

  return SRP_SUCCESS;
}

and

// srp6_server.c, line 98
static SRP_RESULT
srp6_server_params(SRP * srp, const unsigned char * modulus, int modlen,
		   const unsigned char * generator, int genlen,
		   const unsigned char * salt, int saltlen)
{
  unsigned char buf1[SHA_DIGESTSIZE], buf2[SHA_DIGESTSIZE];
  SHA1_CTX ctxt;
  int i;

  /* Fields set by SRP_set_params */

  /* Update hash state */
  SHA1Init(&ctxt);
  SHA1Update(&ctxt, modulus, modlen);
  SHA1Final(buf1, &ctxt);	/* buf1 = H(modulus) */

  SHA1Init(&ctxt);
  SHA1Update(&ctxt, generator, genlen); /* <--------------- same here, g is not padded */
  SHA1Final(buf2, &ctxt);	/* buf2 = H(generator) */

  for(i = 0; i < sizeof(buf1); ++i)
    buf1[i] ^= buf2[i];		/* buf1 = H(modulus) XOR H(generator) */

  /* ckhash: H(N) xor H(g) */
  SHA1Update(&SERVER_CTXP(srp)->ckhash, buf1, sizeof(buf1));

  SHA1Init(&ctxt);
  SHA1Update(&ctxt, srp->username->data, srp->username->length);
  SHA1Final(buf1, &ctxt);	/* buf1 = H(user) */

  /* ckhash: (H(N) xor H(g)) | H(U) */
  SHA1Update(&SERVER_CTXP(srp)->ckhash, buf1, sizeof(buf1));

  /* ckhash: (H(N) xor H(g)) | H(U) | s */
  SHA1Update(&SERVER_CTXP(srp)->ckhash, salt, saltlen);

  return SRP_SUCCESS;
}

yallie added a commit to yallie/Zyan that referenced this issue May 23, 2018
@yallie
Pad g, A and B values according to RFC5054, zyanfx#44.
5e08182 
LinusU/secure-remote-password#12
LinusU/secure-remote-password#13
@LinusU
Copy link
Owner

LinusU commented May 23, 2018

Also, seems like it does pad g when computing k (if the flag SRP_FLAG_LEFT_PAD is set), so it's not that it doesn't support RFC5054 style padding at all.

static SRP_RESULT
srp6a_client_key(SRP * srp, cstr ** result,
		 const unsigned char * pubkey, int pubkeylen)
{
  SRP_RESULT ret;
  BigInteger k;
  cstr * s;
  SHA1_CTX ctxt;
  unsigned char dig[SHA_DIGESTSIZE];

  SHA1Init(&ctxt);
  s = cstr_new();
  BigIntegerToCstr(srp->modulus, s);
  SHA1Update(&ctxt, s->data, s->length);
  if(srp->flags & SRP_FLAG_LEFT_PAD)
    BigIntegerToCstrEx(srp->generator, s, s->length);  /* <================= HERE */
  else
    BigIntegerToCstr(srp->generator, s);
  SHA1Update(&ctxt, s->data, s->length);
  SHA1Final(dig, &ctxt);
  cstr_free(s);

  k = BigIntegerFromBytes(dig, SHA_DIGESTSIZE);
  if(BigIntegerCmpInt(k, 0) == 0)
    ret = SRP_ERROR;
  else
    ret = srp6_client_key_ex(srp, result, pubkey, pubkeylen, k);
  BigIntegerClearFree(k);
  return ret;
}

This makes me even more confident in that we shouldn't pad it when doing H(N) ^ H(g), but should when doing k = H(N, g).

@LinusU LinusU mentioned this issue May 23, 2018
Open
@opencoff
Copy link

opencoff commented May 24, 2018 via email

@idlesign
Copy link

@yallie

I'd say that g should be padded anywhere its value is hashed.
If k uses padded g, then M should also use padded g.

The right thing for libs should be sticking to the RFC, as long as errata is not confirmed and listed in https://www.rfc-editor.org/errata_search.php?rfc=5054. Otherwise the implementation would be something different from srp, and libs won't play well together.
Bear in mind, when in doubt, you can always contact the RFC authors.

@Bouke
Copy link

Bouke commented May 24, 2018 •
edited
Loading

When implementing SRP in Swift for getting it to act as a Homekit device, I used srptools to verify my implementation. It is currently also used for integration tests, replacing pysrp because of incompatibilities. The implementation matches with Apple's implementation of SRP, as it can correctly setup sessions with iOS devices.

Note that there are a few additional implementations targeting Homekit compatibility:

Library k u M H(A, M, K)
go-pkg padded padded not not
fast-srp padded padded not not
Nimbus-SRP padded padded not not

Ideologically it would be very great to achieve compatibility between all SRP implementations, so great initiative going on here!

(table updated)

@LinusU
Copy link
Owner

LinusU commented May 24, 2018

@Bouke I think that fast-srp asserts that the length of A and B is the same as N, so for all intents it's padded...

https://github.com/zarmack/fast-srp/blob/3ca4cef4f2ee7a863550a208f35f2bf97be0ceb5/lib/srp.js#L64-L69

https://github.com/zarmack/fast-srp/blob/3ca4cef4f2ee7a863550a208f35f2bf97be0ceb5/lib/srp.js#L228-L235

The implementation matches with Apple's implementation of SRP, as it can correctly setup sessions with iOS devices.

That's great!

Ideologically it would be very great to achieve compatibility between all SRP implementations, so great initiative going on here!

Yeah, I think it would be awesome 🙌

@yallie
Copy link
Contributor Author

yallie commented May 24, 2018 •
edited
Loading

Perhaps we could keep all SRP implementations in a dedicated github organization.
So it'd be easier to pick a standard-compliant implementation for any given platform.
I can contribute my C# implementation.
It's compatible with @LinusU code and passes the RFC5054 test vectors.

@slechta
Copy link

slechta commented May 24, 2018

@LinusU Pull request are welcome.
I have also written an email to Tom Wu (one of the authors of SRP) with the link to this discussion.

@cocagne
Copy link

cocagne commented May 24, 2018 via email

yallie added a commit to secure-remote-password/srp.net that referenced this issue May 25, 2018
@yallie
Pad g, A and B values according to RFC5054, #44.
5f2fbdc 
LinusU/secure-remote-password#12
LinusU/secure-remote-password#13
@yallie
Copy link
Contributor Author

yallie commented Jun 7, 2018 •
edited
Loading

Organizing a group for compatible SRP implementations would be fantastic.

I've created an organization for RFC5054-compliant implementations: secure-remote-password.
Also, I've created a repository for test vectors in JSON format: test-vectors.

SHA1 test vectors are taken from the RFC5054 document.
Additional test vectors are generated using srptools by @idlesign.

If anyone is interested in contributing a compatible SRP implementation, please let me know.
Thanks!

@idlesign
Copy link

idlesign commented Jun 8, 2018

Hi,

I think it would be great if compatible repos are not transfered into organization alltogether but rather listed in some repository under org (say secure-remote-password/implementations), so that on the one hand maintainers could subscribe to issues, and end-users will see some kind of readme with a library listing table (maybe even with badges version, CI, etc.).

So that:

  • New libraries are added into table using pull requests.
  • Maintainers are added into organization.
  • Problems and similar would go into Issues.
  • Whenever there's a confirmed issue with a library, README table is updated accordingly (library line is marked as having problem).

@opencoff
Copy link

opencoff commented Jun 8, 2018 via email

@yallie
Copy link
Contributor Author

yallie commented Jun 8, 2018 •
edited
Loading

@idlesign

I think it would be great if compatible repos are not transfered into organization alltogether but rather listed in some repository under org (say secure-remote-password/implementations)

I believe both options are just fine. My account is mostly used for personal projects, so I'd rather host my library in the org account and have all SRP-related issues/discussions in a single place.

@opencoff

Good point! But most libraries mentioned here can use custom parameters and/or hash functions. When set up using SHA1 and 1024-bit group, my library passes the RFC5054 test vectors, but the default parameters are different from those suggested by the RFC.

My goal is just to have a list of libraries that can connect to each other, not to enforce strict RFC5054 compliance by default.

@idlesign
Copy link

idlesign commented Jun 9, 2018

I've started https://github.com/secure-remote-password/implementations
Contributions are welcome.

@cocagne
Copy link

cocagne commented Jun 10, 2018 via email

@LinusU
Copy link
Owner

LinusU commented Jun 18, 2018

I'd be happy to move this implementation into the new org as well! As soon as we've figured out exactly how we all want to be compatible.

cocagne/pysrp#38 raised some interesting concerns about padding g when calculating M.

@Bouke it seems like your library doesn't pad g, and is using a BigInt for g and not raw bytes. That should mean that Apple doesn't pad g, right? 🤔

@yallie yallie mentioned this issue Dec 5, 2019
Closed
@LinusU
Copy link
Owner

LinusU commented Feb 12, 2020

Hey everyone 👋

Sorry for not moving forward with this in a very long time. I would really like to figure out if we should pad g before hashing it or not 🤔

If anyone has any information to chime in with I would be very happy to hear it!

@LinusU LinusU mentioned this issue Feb 13, 2020
Closed
@zbarbuto
Copy link

Copying my findings from #24:

I've tested this library on its own as per the docs and can see that it will correctly verify the client as expected. However, I've tried passing values between this and other SRP libraries and the values seem not to verify (while they would when passing between the other libraries alone).

This includes:

  • using jsrp as the client and secure-remote-password as the sever (and vice versa)
  • using Ruby's sirp as the sever and secure-remote-password as the client
  • using Ruby's sirp as the sever and jsrp as the client

I've found that while sirp and jsrp will happily communicate and verify against each other, secure-remote-password will not verify client values from jsrp configured in 2048 mode and sirp on the server will not verify client values from secure-remote-password. I've yet to get secure-remote-password to successfully work with another library.

After much digging, the only thing I can find that might be causing the issue is that the k values being used are different. I compared all the hex values form both calculations of a B value (secure-remote-password's const B = k.multiply(v).add(g.modPow(b, N)).mod(N) and jsrp's this.k().multiply(v).add(this.params.g.modPow(b, this.params.N)).mod(this.params.N) and all hex values were identical expect the k value.

It's strange because the calculations seem to be the same:

secure-remote: exports.k = sha256(exports.N, exports.g)

vs

jsrp: createHash(this.params.hash).update(transform.pad.toN(this.params.N, this.params)).update(transform.pad.toN(this.params.g, this.params)).digest();

Both use the same values for N and g. The only difference seems to be that jsrp will pad the g value to the same length as the N value in the hash.

k value from jsrp:

5b9e8ef059c6b32ea59fc1d322d37f04aa30bae5aa9003b8321e21ddb04e300

k value from srp-js (a fork of node-srp by mozilla):

5b9e8ef059c6b32ea59fc1d322d37f04aa30bae5aa9003b8321e21ddb04e300

k value from secure-remote-password:

4cba3fb2923e01fb263ddbbb185a01c131c638f2561942e437727e02ca3c266d

@space55
Copy link

space55 commented Apr 30, 2021

Hey, just wanted to chime in. I'm looking to use this library against Python's srptools, which it is currently incompatible with. Is there any update on making everything work together? Maybe a configuration option which allows you to choose what to pad?

Thanks!

@cocagne
Copy link

cocagne commented Apr 30, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

💥 Pad values following RFC5054 LinusU/secure-remote-password
9 participants
@idlesign @LinusU @Bouke @yallie @slechta @cocagne @space55 @opencoff @zbarbuto