fix: AuthenticationHelper - Handle negative BigIntegers

[manueliglesias]

Using two's complement for negative numbers

Issue #, if available:

Description of changes:
Use two's complement representation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[lgtm-com]

This pull request introduces 1 alert when merging 1f36227 into 3ce644c - view on LGTM.com

new alerts:

• 1 for Assignment to constant

[codecov]

Codecov Report

Merging #7618 (cbd9b63) into main (3e423d0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #7618   +/-   ##
=======================================
  Coverage   74.07%   74.07%           
=======================================
  Files         214      214           
  Lines       13410    13410           
  Branches     2628     2628           
=======================================
  Hits         9934     9934           
  Misses       3277     3277           
  Partials      199      199           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e423d0...cbd9b63. Read the comment docs.

[lgtm-com]

This pull request introduces 1 alert when merging 778ad79 into 7b5b23f - view on LGTM.com

new alerts:

• 1 for Assignment to constant

[lgtm-com]

This pull request introduces 1 alert when merging e955f03 into 7b5b23f - view on LGTM.com

new alerts:

• 1 for Assignment to constant

[lgtm-com]

This pull request introduces 1 alert when merging ce598ac into 651c3b2 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging 3c9005e into dff7994 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[Jeff17Robbins]

I apologize for jumping in here, but I'm confused about the very idea of a "negative" hex string. My understanding is that Cognito was erroneously returning a "salt" value as a hex string preceded by a hyphen. I don't think the right fix is to ingest that salt value. The existing JavaScript code was already ignoring the hyphen "by luck", but I didn't think that the hyphen prefix was there intentionally, but, rather, it was a bug.

The reason I think the JavaScript code should assert an error if confronted with a broken hex string is that then someone would have noticed the (arguably bad) Cognito bug sooner.

Again, if I am misinformed about the nature of this PR, I apologize in advance!

[manueliglesias]

Hi @Jeff17Robbins

To give some additional context, the idea of a negative hex string is built into the BigInteger implementation used in this client and Java's BigInteger class. Cognito was returning a value compatible with the BigInteger.toString() of a negative number, which in and of itself is not invalid, it's just how BigInteger encodes negative numbers. However, this client was not properly parsing this format. This PR updates the client to properly parse this format. Although Cognito doesn't return negative salt values anymore, other places in the client use this format, and we need to ensure compatibility between the JS client and Cognito.

[Jeff17Robbins]

Hi @manueliglesias,

I think maybe there are two topics here.

I am under the impression that the SRP salt cannot properly be a negative hex string. (Although to be honest, I can't prove my impression is correct!) But presumably, there's some "fact of the matter" when it comes to that security protocol. If the salt cannot be a negative hex string, then the Cognito client shouldn't silently ignore an invalid SRP parameter.

My point here is about the problem of silently accepting erroneous protocol from Cognito. In this instance, Cognito offered an unfortunate "tell" whereby I could check the validity of the username merely from examining the salt value, without needing to execute the relatively slow SRP math which normally would deter a hacker from a dictionary attack. Had the receiving code recognized the "negative" hex string as invalid, it could have thrown an error, thereby alerting the community that there was a real problem in Cognito's SRP implementation.

So I think we should determine if the salt value in SRP is allowed to be a "negative" hex string. If it is, then I agree with you on this. If it isn't, then I don't think it is correct to parse it with code that accepts an invalid value.

Perhaps the devs who work on the Cognito SRP implementation, who fixed their end of this bug, could be approached as to what the protocol demands?

As to other uses for negative hex strings: this is new to me (as a somewhat old timer who came up from machine code/assembly language where there really was no such thing. ) Negative numbers when represented in hex simply had the high bit set of the top byte, in those days. Of course I believe you regarding the support for this feature in the two libraries you cite! I am curious what the use-case is for that notation. If you have any pointers for further reading, I'd like to learn more!

Cognito was returning a value compatible with the BigInteger.toString() of a negative number, which in and of itself is not invalid

[crockeea]

I am under the impression that the SRP salt cannot properly be a negative hex string.

A salt value is just a string of bytes, it doesn't really have a sign, and there is nothing wrong with a negative salt value from the protocol's perspective. It is not incorrect for Cognito to send such a value to the client, as long as it is properly handled in the client.

I am curious what the use-case is for that notation.

I couldn't say, other than that that is how BigInteger chooses to serialize numbers in hex (as opposed to two's complement).

[Jeff17Robbins]

@crockeea,

re

A salt value is just a string of bytes, it doesn't really have a sign...It is not incorrect for Cognito to send such a value to the client

I wanted to make sure my reasoning is clear, so please be patient with the somewhat lengthy exposition presented here:

The original SRP paper RFC 2945 uses a non-negative number for its salt. I base this on the fact that the salt value is the output of the random() function in the paper's pseudo-code. This same random() function is used for b, and since g is raised to the bth power, we can see that the random() function doesn't return a negative number.

Since the encoding of integers into hex strings is specified also, there's no way that SRP's salt could correctly be a hex string preceded by a hyphen, or a "negative hex string", when represented in the protocol.

Therefore, it is incorrect for Cognito to send such a value, in this instance.

I understand that the BigInteger software construct supports such "negative hex strings", but, again, since the SRP salt cannot properly ever be such a thing, a client SRP algorithm shouldn't silently accept it.

Again, I think it would be better to check the above reasoning with the Cognito team. After all, they recently fixed a bug in Cognito's SRP code, that produced the negative sign in front of the salt hex string in the case of an unknown user name. The erroneous behavior of this bug should not be taken as evidence that the salt can be "negative". Cognito no longer returns erroneous negative salt hex strings. If it does, it will be due to another bug, and the JavaScript client should sound the alarm.

Given the sensitive nature of cybersecurity, I think silently accepting invalid security protocol parameters (if my reasoning above is correct) is of high concern, and shouldn't be influenced by the behavior of the BigInteger library. Based on my reading of the SRP specification, no hex string presented by the server (Cognito) will ever have a negative sign in front of it.

[paulie4]

A salt value is just a string of bytes, it doesn't really have a sign, and there is nothing wrong with a negative salt value from the protocol's perspective.

Since bytes cannot be negative, it is therefore NOT valid for a hexadecimal string that represents those bytes to have a negative symbol. If this was just a regular hexadecimal number, then it could be negative, but this is a representation of bytes, so it cannot be negative.

[amhinson]

Look good to me! I'd also suggest testing React Native iOS & Android as well as Expo, since they all have different underlying implementations of BigInteger.

[Jeff17Robbins]

If Cognito erroneously returns a hyphen in front of its salt hex string (a "negative hex string"), will this code process it as if it were correct, or will it alert us to an error in Cognito?

[github-actions]

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.