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.

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

Calculation of B #13

Open
dvj opened this issue Mar 21, 2016 · 5 comments
Open

Calculation of B #13

dvj opened this issue Mar 21, 2016 · 5 comments

Comments

@dvj
Copy link

dvj commented Mar 21, 2016

It seems there is a difference between the python and C implementation of how B is calculated, notably that in the C version:

            # B = kv + g^b
            BN_mul(self.tmp1, k, self.v, self.ctx)
            BN_mod_exp(self.tmp2, g, self.b, N, self.ctx)
            BN_add(self.B, self.tmp1, self.tmp2)

the modulo differs from the python version:

    self.B = (k*self.v + pow(g, self.b, N)) % N

Should that be BN_mod_add() instead?

@cocagne
Copy link
Owner

cocagne commented Mar 21, 2016

Possibly. Are you asking because you're trying to root-cause a problem?
There have been a few bugs related to mod operations in the past but I
haven't heard of one in quite a while. That would suggest that the current
implementation is correct but it's far from a guarantee...

Tom

On Mon, Mar 21, 2016 at 12:05 AM, Doug Johnston notifications@github.com
wrote:

It seems there is a difference between the python and C implementation of
how B is calculated, notably that in the C version:

        # B = kv + g^b
        BN_mul(self.tmp1, k, self.v, self.ctx)
        BN_mod_exp(self.tmp2, g, self.b, N, self.ctx)
        BN_add(self.B, self.tmp1, self.tmp2)

the modulo differs from the python version:

self.B = (k*self.v + pow(g, self.b, N)) % N

Should that be BN_mod_add() instead?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#13

@dvj
Copy link
Author

dvj commented Mar 21, 2016

I'm trying to interop with another SRP implementation, which wasn't working, and debugging. Specifically, I was seeing different key lengths for A and B. This doesn't happen with the default ng of 2048, but using 4096 as an example:

    salt, vkey = srp.create_salted_verification_key('testuser', 'testpassword', ng_type=srp.NG_4096)
    usr = srp.User('testuser', 'testpassword', ng_type=srp.NG_4096)
    uname, A = usr.start_authentication()
    svr = srp.Verifier(uname, salt, vkey, A, ng_type=srp.NG_4096)
    s, B = svr.get_challenge()

which yields a key length of 512 for A, but a key length of 531 for B using the C implementation.

@cocagne
Copy link
Owner

cocagne commented Mar 21, 2016

Okay, that makes sense. pysrp & csrp were not originally designed to
interoperate with other SRP implementations. So far as I know, there is no
official "this is the standard way to implement SRP in general" but
apparently there are several libraries that adhere to the SSL-defined
implementation approach (not sure which ones those are but there's
definitely more than one). There's an rfc5054_compat branch in both the
pysrp & csrp git repositories that match the RFC5054 implementation
requirements. Several people have had good luck interoperating with other
implementations when using that branch so it's probably what you're looking
for. At some point I'll probably merge that branch into the mainline and
make it the default implementation.

Tom

On Mon, Mar 21, 2016 at 11:22 AM, Doug Johnston notifications@github.com
wrote:

I'm trying to interop with another SRP implementation, which wasn't
working, and debugging. Specifically, I was seeing different key lengths
for A and B. This doesn't happen with the default ng of 2048, but using
4096 as an example:

salt, vkey = srp.create_salted_verification_key('testuser', 'testpassword', ng_type=srp.NG_4096)
usr = srp.User('testuser', 'testpassword', ng_type=srp.NG_4096)
uname, A = usr.start_authentication()
svr = srp.Verifier(uname, salt, vkey, A, ng_type=srp.NG_4096)
s, B = svr.get_challenge()

which yields a key length of 512 for A, but a key length of 531 for B
using the C implementation.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#13 (comment)

@dvj
Copy link
Author

dvj commented Mar 22, 2016

I've now gotten things to work with the other SRP implementation using the rfc5054_compat branch. It does appear as if that line is now using a mod_add call, amongst a couple hashing changes.

I would highly encourage merging that with master, as the branch has several bugs and missing features. There was some hesitation in #11 to make it the default for backwards compatibility reasons, but I think looking at the long-term view of this module, using RFC5054 is the right thing to do. Besides, I doubt too many people have been waiting with baited breath since the last pypi release for a point release. :)

Now that I've become intimately familiar with the code, I may have a feature or two to add, including variable salt length, separating the private key generation and adding in other standard Ng options; all of which I needed to implement to use this module with the scheme I'm implementing against.

In the meantime, I would also suggest adding a note to the readthedocs page indicating that it's not RFC5054, as that point is not clear currently.

Thanks for the work.

@cocagne
Copy link
Owner

cocagne commented Mar 24, 2016

Thanks Doug. I agree with you on every point. The only reason I haven't
already merged the rfc5054 branch over into the mainline is that it lacks
backwards compatibility support. I agree that hardly anyone is eagerly
awaiting the next release but I do suspect that quite a few people would be
irked if new pip installs suddenly started breaking applications. It's not
a difficult problem to solve, just time-consuming. Which is rather pathetic
to say since time-consuming in this case probably amounts to about 8 hours
of total effort between pysrp & csrp. When I originally wrote py/csrp, I
had several use-cases in mind for production deployment. Unfortunately, I
wound up changing jobs at approximately the same time as the original
release and I have yet to find a good reason for using py/csrp at my
current position. So I'm left maintaining these packages but not personally
benefiting from them, other than helping out the OSS community that is.
That's a plus, certainly, but the evidence would suggest it's also a
lackluster motivator. Between family time, sleep, and srp... Well, I
suppose it's obvious where my time has been going.

That said, I'd be very interested in any changes you'd like to contribute
up stream. It's a lot easier to spend time on this kind of project when you
know it's of real benefit to an actual person, not just some hypothetical
user that might someday be interested.

Tom

On Mon, Mar 21, 2016 at 10:11 PM, Doug Johnston notifications@github.com
wrote:

I've now gotten things to work with the other SRP implementation using the
rfc5054_compat branch. It does appear as if that line is now using a
mod_add call, amongst a couple hashing changes.

I would highly encourage merging that with master, as the branch has
several bugs and missing features. There was some hesitation in #11
#11 to make it the default for
backwards compatibility reasons, but I think looking at the long-term view
of this module, using RFC5054 is the right thing to do. Besides, I doubt
too many people have been waiting with baited breath since the last pypi
release for a point release. :)

Now that I've become intimately familiar with the code, I may have a
feature or two to add, including variable salt length, separating the
private key generation and adding in other standard Ng options; all of
which I needed to implement to use this module with the scheme I'm
implementing against.

In the meantime, I would also suggest adding a note to the readthedocs
page indicating that it's not RFC5054, as that point is not clear currently.

Thanks for the work.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#13 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants