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

add SRP authenticaton support #4

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@nakagami
Contributor

nakagami commented Aug 19, 2015

SRP authentication (protocol version13) support.
But not WireCrypt.

private static BigInteger getUserHash(String user, String password, byte[] salt) {
final byte[] hash1 = sha1(user.getBytes(), ":".getBytes(), password.getBytes());

This comment has been minimized.

@mrotteveel

mrotteveel Aug 19, 2015

Member

Not entirely sure, but I think this should use UTF-8 (as in getBytes(StandardCharsets.UTF_8)).

We will also need to handle username or password being null (although not necessarily here).

This comment has been minimized.

@AlexPeshkoff

AlexPeshkoff Aug 19, 2015

Member

On 08/19/2015 04:41 PM, Mark Rotteveel wrote:

  •    byte[] b = new byte[SRP_KEY_SIZE/8];
    
  •    SecureRandom random = new SecureRandom();
    
  •    random.nextBytes(b);
    
  •    return fromBigByteArray(b);
    
  • }
  • private static byte[] getSalt() {
  •    byte[] b = new byte[SRP_SALT_SIZE];
    
  •    SecureRandom random = new SecureRandom();
    
  •    random.nextBytes(b);
    
  •    return b;
    
  • }
  • private static BigInteger getUserHash(String user, String password, byte[] salt) {
  •    final byte[] hash1 = sha1(user.getBytes(), ":".getBytes(), password.getBytes());
    
    Not entirely sure, but I think this should use UTF-8 (as in getBytes(StandardCharsets.UTF_8)

Password should be in UTF8 charset for correct calculations.

public final class SrpClient {
private static final int SRP_KEY_SIZE = 128;
private static final int SRP_SALT_SIZE = 32;
private static final BigInteger N = new BigInteger("E67D2E994B2F900C3F41F08F5BB2627ED0D49EE1FE767A52EFCD565CD6E768812C3E1E9CE8F0A8BEA6CB13CD29DDEBF7A96D4A93B55D488DF099A15C89DCB0640738EB2CBDD9A8F7BAB561AB1B0DC1C6CDABF303264A08D1BCA932D1F1EE428B619D970F342ABA9A65793B8B2F041AE5364350C16F735F56ECBCA87BD57B29E7", 16);

This comment has been minimized.

@mrotteveel

mrotteveel Aug 19, 2015

Member

One character constants are not very readable. Do you know what these values stand for, or is this the same in the Firebird implementation?

This comment has been minimized.

@nakagami

nakagami Aug 19, 2015

Contributor

http://srp.stanford.edu/design.html
N: A large safe prime
g: A generator modulo N
k: Multiplier parameter (k = H(N, g) in SRP-6a, k = 3 for legacy SRP-6)

Firebird core source declare as "prime", "generator" and "k".
https://github.com/nakagami/core/blob/master/src/auth/SecureRemotePassword/srp.cpp#L13
I think they are not so meaningfull.

private static BigInteger getSecret() {
byte[] b = new byte[SRP_KEY_SIZE/8];
SecureRandom random = new SecureRandom();

This comment has been minimized.

@mrotteveel

mrotteveel Aug 19, 2015

Member

Declare the random generator once on the class level

@mrotteveel

This comment has been minimized.

Member

mrotteveel commented Aug 19, 2015

Thanks, I will check it in more detail this weekend.

Were you also planning to work on the wire encryption?

@nakagami

This comment has been minimized.

Contributor

nakagami commented Aug 20, 2015

Yes, I will work about wire encryption, after this patch merged.

}
private static BigInteger fromBigByteArray(byte[] b) {
return new BigInteger(DatatypeConverter.printHexBinary(b), 16);

This comment has been minimized.

@mrotteveel

mrotteveel Aug 22, 2015

Member

Would it be possible to use the BigInteger(byte[]) constructor here instead, or are you trying to avoid negative BigInteger values? In that case new BigInteger(int signum, byte[] magnitude) can be used This will prevent the String conversion step.

This comment has been minimized.

@nakagami

nakagami Aug 23, 2015

Contributor

Sorry I didn't know the constructor, and I don't know which is better.
This conversion need keep positive, But I did'nt try BigInteger(int signum, byte[] magnitude) constructor, so I don't know the constructor keep positive or not.

byte[] b = new byte[SRP_KEY_SIZE/8];
SecureRandom random = new SecureRandom();
random.nextBytes(b);
return fromBigByteArray(b);

This comment has been minimized.

@mrotteveel

mrotteveel Aug 22, 2015

Member

Is there a reason not to use new BigInteger(int numBits, Random random)?

This comment has been minimized.

@nakagami

nakagami Aug 23, 2015

Contributor

There is no reason, but only I did'nt know that constructor.
And I don't know which is better.

I wrote draft codes now, and you can modify thease BigInteger and byte[] treatment as you like.

@mrotteveel

This comment has been minimized.

Member

mrotteveel commented Aug 22, 2015

@nakagami Could you take a look at my comments regarding the use of other BigInteger constructors; I think it shouldn't be a problem to use those, but maybe you intentionally didn't use them.

@mrotteveel

This comment has been minimized.

Member

mrotteveel commented Aug 22, 2015

I get random "Your user name and password are not defined. Ask your database administrator to set up a Firebird login." errors for +/- 30 out of 2400 tests (the failing tests are random), this seems to indicate that something is not entirely correct with how SRP is handled.

spb.addArgument(isc_spb_password, props.getPassword(), encoding);
}
if (props.getRoleName() != null) {
spb.addArgument(isc_spb_sql_role_name, props.getRoleName(), encoding);
}
if (props.getAuthData() != null) {
spb.addArgument(isc_dpb_specific_auth_data, DatatypeConverter.printHexBinary(props.getAuthData()), encoding);

This comment has been minimized.

@mrotteveel

mrotteveel Aug 22, 2015

Member

Must use isc_spb_specific_auth_data instead

This comment has been minimized.

@nakagami

nakagami Aug 23, 2015

Contributor

fix it, and pushed.

@nakagami

This comment has been minimized.

Contributor

nakagami commented Aug 23, 2015

I have add a few commit and push them.
Obvious serious problems are probably fixed.

@mrotteveel

This comment has been minimized.

Member

mrotteveel commented Aug 23, 2015

Thanks, I will see if I can use the BigInteger constructors, as it skips some conversion steps.

@mrotteveel

This comment has been minimized.

Member

mrotteveel commented Aug 23, 2015

I merged the changes including some changes based on my review. Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment