RFC5054 - Fixed an issue when calculating M, producing bad values

[macmade]

When calculating M, RFC5054 requires the generator g to be padded.
This is handled inside srp_user_process_challenge by using H_nn_rfc5054.
However, padding was missing in calculate_M, since hashing is done differently, with an xor.

[cocagne]

Thanks for the patches JD, a cursory glance at them looks good and I'll take a deeper look this weekend. One point on his patch though, did you see errors with csrp interacting with other SRP implementations that caused you to change the padding for the M calculation? The reason I ask is that csrp has been stable for quite some time now so I want to make sure we're fixing an actual problem, not just a theoretical one. Tom

…

On Wed, Nov 6, 2024 at 8:49 AM JD Gadina ***@***.***> wrote: When calculating M, RFC5054 requires the generator g to be padded. This is handled inside srp_user_process_challenge by using H_nn_rfc5054. However, padding was missing in calculate_M, since hashing is done differently, with an xor. ------------------------------ You can view, comment on, or merge this pull request online at: #25 Commit Summary - 14b570e <14b570e> fix(RFC5054): Fixed an issue when calculating M. RFC5054 requires generator g to be padded (2.6). File Changes (1 file <https://github.com/cocagne/csrp/pull/25/files>) - *M* srp.c <https://github.com/cocagne/csrp/pull/25/files#diff-5144536c1faa90eaba1e97de54e59f36ab3555917f984537d58fbcddbffa01ad> (32) Patch Links: - https://github.com/cocagne/csrp/pull/25.patch - https://github.com/cocagne/csrp/pull/25.diff — Reply to this email directly, view it on GitHub <#25>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANMW7FF3TAW34665HU4VHDZ7IT7LAVCNFSM6AAAAABRJAOZWCVHI2DSMVQWIX3LMV43ASLTON2WKOZSGYZTQMZRGIZTGMY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[macmade]

Thanks for the reply, Tom.

My pull requests follow changes in Apple’s iCloud authentication.
You may have seen discussions around that on some other projects using your Python library, such as pyiCloud:
picklepete/pyicloud#456

Our software, iMazing, uses an endpoint on iCloud to allow users to retrieve high-resolution photos if they’re unavailable on the device.

iCloud authentication is now SRP6a, with extra derivation on the password using PBKDF2.

We replaced our old implementation with your C library. This required a few changes, such as:

• An option to not hash the username when calculating X.
• A way to set the password after the SRPUser structure has been created since the server communicates PBKDF2 iterations along with the salt.
• A way to retrieve H_AMK from the SRPUser structure since the server requires M2 to be passed with M1.

These were already present in the Python version.

Even after these changes, authentication still did not work on our side.
So, I compared the Python and C implementations. I used fixed values for a, b, salt, etc., and noticed the value of M1 was different between the two.

Adding padding in G solved the issue; the two implementations now gave the same result for M1.
Testing the whole process again, iCloud authentication was also now possible.

I hope this clarifies the reason for my pull requests.
Don't hesitate if you need more details.

[cocagne]

I thought this might have something to do with the iCloud issue. The projects have been quiet for years and then there's a sudden flurry of activity. Thanks for the patch sets, they all look great and thanks for the explanation. I figured you had real reasons for modifying the hashing algorithm (why bother if you didn't) but it's reassuring to know there's a valid issue with it. Tom

…

On Fri, Nov 8, 2024 at 4:58 AM JD Gadina ***@***.***> wrote: Thanks for the reply, Tom. My pull requests follow changes in Apple’s iCloud authentication. You may have seen discussions around that on some other projects using your Python library, such as pyiCloud: picklepete/pyicloud#456 <picklepete/pyicloud#456> Our software, iMazing, uses an endpoint on iCloud to allow users to retrieve high-resolution photos if they’re unavailable on the device. iCloud authentication is now SRP6a, with extra derivation on the password using PBKDF2. We replaced our old implementation with your C library. This required a few changes, such as: - An option to not hash the username when calculating X. - A way to set the password after the SRPUser structure has been created since the server communicates PBKDF2 iterations along with the salt. - A way to retrieve H_AMK from the SRPUser structure since the server requires M2 to be passed with M1. These were already present in the Python version. Even after these changes, authentication still did not work on our side. So, I compared the Python and C implementations. I used fixed values for a, b, salt, etc., and noticed the value of M1 was different between the two. Adding padding in G solved the issue; the two implementations now gave the same result for M1. Testing the whole process again, iCloud authentication was also now possible. I hope this clarifies the reason for my pull requests. Don't hesitate if you need more details. — Reply to this email directly, view it on GitHub <#25 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANMW7EMFGKT7BVPOFUZYOTZ7SKN5AVCNFSM6AAAAABRJAOZWCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRUGQYDOOBYGU> . You are receiving this because you commented.Message ID: ***@***.***>

[macmade]

Thank you very much for merging these, and for your library!
Wish you a good weekend!