secure-remote-password / srp.net

SRP-6a protocol implementation for .NET Standard 1.6+ and .NET Framework 3.5+
https://www.nuget.org/packages/srp
MIT License
64 stars 10 forks source link

Should the `Generator` (`g`) be padded in the `M` calculation for SrpServer? #19

Closed mrhappyasthma closed 2 years ago

mrhappyasthma commented 2 years ago

I'm trying to port an SRP implementation over to C# from python. The client is legacy and I cannot change it, so I'm just implementing a compatible server side of the protocol.

It was failing using this library until I noticed that the library doesn't Pad the g value here: https://github.com/secure-remote-password/srp.net/blob/master/src/srp/SrpServer.cs#L130

(It doesn't pad in the SrcClient either. So it's consistent. But I'm wonder if we should update both?)

The python SRP library does pad this value when enabling support for rfc5054: https://github.com/cocagne/pysrp/blob/master/srp/_pysrp.py#L205-L208

I tried jumping through the rfc5054 docs, but it doesn't even mention the M proof calculation so it's unclear if the padding should also apply there. It mentions padding k and u (which this library does) but doesn't claim one way or the other for M: https://datatracker.ietf.org/doc/html/rfc5054

I was able to workaround this issue by doing the following:

SrpParameters parameters = SrpParameters.Create<SHA256>(N, g);
parameters.Generator = parameters.Pad(parameters.Generator);  // WORKAROUND
SrpServer srpServer = new SrpServer(parameters);
SrpSession serverSession = srpServer.DeriveSession((.........);

It works for my needs. But I guess the larger question for this issue is: Should this library actually be padding g in the M calculation? Or is the python srp implementation incorrect?

yallie commented 2 years ago

Hello Mark, thanks for posting your workaround.

This library aims to be interoperable with other standard-compliant implementations. You're right, RFC5054 doesn't specify how exactly M value is calculated. But looks like most implementations don't pad g when calculating M proof value.

Srp.net is compatible with Swift SRP which is able to connect to Apple's servers. I don't think that we should change the current behavior, but we can add a note about compatibility workaround for your python client implementation, if it's open source.

mrhappyasthma commented 2 years ago

The python API is open source, it's the main srp implementation there. But the client I'm working with is actually using some C++ library. So presumably there's more than just 1 client doing things this way.

We could consider making it easier to configure the padding of g in the M calculation ,but since a workaround exists I don't think it's necessary. Perhaps we can just make a comment for future folks who may run into the same problem working with an alternate srp implementation.

What do you think?

yallie commented 2 years ago

Hello Mark, sorry for my late response.

SRP protocol has several obsolete revisions, and of course it makes sense to support them if they are still in use somewhere. I don't think however that the library should encourage the use of old revisions because they are less secure than the latest SRP-6a, so I believe that your workaround is just fine.

Perhaps we can just make a comment for future folks who may run into the same problem working with an alternate srp implementation.

It would be great if you add a couple of lines to the readme about your workaround and when it should be needed. There is a compatibility section that already has a few tips on that matter.

Or maybe implementations repo is a better place to mention partially-compatible libraries.

Thank you!

mrhappyasthma commented 2 years ago

Sounds like a plan. When I get some downtime I'll update the docs.