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

v4.0.0 include identity by default #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 39 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,6 @@ SRP alone only prevents a man-in-the-middle attack from _reading_ the password,

Always use SRP in combination with HTTPS. Browsers can be vulnerable to: having malicious certificates installed beforehand, rogue certificates in the wild, server misconfiguration, bugs like the heartbleed attack, servers leaking password into errors and logs. SRP in the browser offers an additional hurdle and may prevent some mistakes from escalating.

The client can choose to exclude the identity of its computations or not. If excluded, the id cannot be changed. But this problem is better solved by an application schema that separates "identity" from "authentication", so that one identity can have multiple authentications. This allows to switch identity + password, and also to user more than one way of logging in (think "login with email+password, google, or facebook").
bgrosse-midokura marked this conversation as resolved.
Show resolved Hide resolved

## Serialization

The SRP protocol and therefore this library is stateful. Each step sets various internal state. Due to the randomness of some of this state (namely the public and private values), repeating the step methods with the same arguments is unlikely (almost definitely) to result in the same state. This proves to be an issue when using a stateless protocol such as HTTP (as opposed to websockets). The server "session" state (the server step 1 state) might not be easily kept in memory. Therefore, we provide a way to serialize and deserialize the step classes in order to restore state. [serialize.test.ts](test/serialize.test.ts) shows some examples here's an explanation of how it works:
Expand Down Expand Up @@ -144,19 +142,48 @@ Supported steps/classes for serialization are:

While the password is **never** kept directly in the state, hashes of it are. If an adversary is able to access the serialized state it will likely open you up to some kind of MITM attack and depending on the step, may allow an attacker to perform a bruteforce and/or dictionary attack to retrieve the password. **Do not expose the serialized data.** For clients, this means do not send it over the network and be careful where you store it. For servers, only send it in encrypted form to parties you trust (such as your database). If you believe state at anytime may have been exposed, it is suggested you change passwords as soon as possible.

## Notes
## Identity and Nimbus implementation

This package's default configuration matches SRP6a: the identity is included in the verifier generation. This makes it impossible to detect if [two users share the same password](https://crypto.stackexchange.com/questions/8626/why-is-tls-srp-verifier-based-on-user-name/9430#9430) but also does not allow a client to change its "identity" without regenerating password.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there more vulnerabilities because of that or this is the only known one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's the only known problem.


But this problem is better solved by an application schema that separates "identity" from "authentication". In general, this isolation has more benefits like allowing multiple authentication methods for an account. The "authentication id" could then be used as "identity" in SRP6a computations, as long as it unique and belongs to exactly one user. The user's "identity" could be changed without impacting the linked authentication.

This package's default configuration matches the following Java's
[Nimbus SRP](https://connect2id.com/products/nimbus-srp) configuration:
To follow Java's [Nimbus SRP](https://connect2id.com/products/nimbus-srp) configuration, which does NOT include the identity in computations:
```Java
SRP6CryptoParams.getInstance(2048, "SHA-512")
```

The default routines does not
strictly follow SRP6a RFC because user identity is NOT included in the verifier generation.
This makes possible for malicious server to detect if
[two users share the same password](https://crypto.stackexchange.com/questions/8626/why-is-tls-srp-verifier-based-on-user-name/9430#9430)
but also allows client to change it "identity" without regenerating password.
```TypeScript
import {
SRPRoutines,
arrayBufferToBigInt,
bigIntToArrayBuffer,
stringToArrayBuffer,
} from "tssrp6a";

class NimbusRoutines extends SRPRoutines {
public async computeIdentityHash(
_I: string,
P: string,
): Promise<ArrayBuffer> {
return await this.hash(stringToArrayBuffer(P));
}
public async computeClientEvidence(
_I: string,
bufistov marked this conversation as resolved.
Show resolved Hide resolved
_s: bigint,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

A: bigint,
B: bigint,
S: bigint,
): Promise<bigint> {
return arrayBufferToBigInt(
await this.hash(
bigIntToArrayBuffer(A),
bigIntToArrayBuffer(B),
bigIntToArrayBuffer(S),
),
);
}
}
```

[This example](test/srp6a.test.ts) shows how to make SRP client strictly compliant with
SRP6a specification.
See [the nimbus test](test/nimbus_compatibility.test.ts) for reference.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "tssrp6a",
"version": "3.0.0",
"version": "4.0.0",
"main": "dist/index.js",
"files": [
"dist/**/*",
Expand Down
10 changes: 6 additions & 4 deletions src/routines.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ export class SRPRoutines {
);
}

public async computeIdentityHash(_: string, P: string): Promise<ArrayBuffer> {
return await this.hash(stringToArrayBuffer(P));
public async computeIdentityHash(I: string, P: string): Promise<ArrayBuffer> {
return await this.hash(stringToArrayBuffer(I), stringToArrayBuffer(P));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC adds ':' between identity and password, I think:

SHA1(I | ":" | P)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok let's follow that

}

public computeVerifier(x: bigint): bigint {
Expand Down Expand Up @@ -108,14 +108,16 @@ export class SRPRoutines {
}

public async computeClientEvidence(
_I: string,
_s: bigint,
I: string,
s: bigint,
A: bigint,
B: bigint,
S: bigint,
): Promise<bigint> {
return arrayBufferToBigInt(
await this.hash(
stringToArrayBuffer(I),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WHy is this needed? I think RFC only says hash A and B values for evidence. Notice that we can use any known to both client and server values here without impacting anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know, so I will change it back.

bigIntToArrayBuffer(s),
bigIntToArrayBuffer(A),
bigIntToArrayBuffer(B),
bigIntToArrayBuffer(S),
Expand Down
37 changes: 33 additions & 4 deletions test/nimbus_compatibility.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,42 @@ import { SRPParameters } from "../src/parameters";
import { SRPRoutines } from "../src/routines";
import { SRPClientSession } from "../src/session-client";
import { SRPServerSession } from "../src/session-server";
import { createVerifier } from "../src/utils";
import {
arrayBufferToBigInt,
bigIntToArrayBuffer,
createVerifier,
stringToArrayBuffer,
} from "../src/utils";
import { test } from "./tests";

class NimbusRoutines extends SRPRoutines {
public async computeIdentityHash(
_I: string,
P: string,
): Promise<ArrayBuffer> {
return await this.hash(stringToArrayBuffer(P));
}
public async computeClientEvidence(
_I: string,
_s: bigint,
A: bigint,
B: bigint,
S: bigint,
): Promise<bigint> {
return arrayBufferToBigInt(
await this.hash(
bigIntToArrayBuffer(A),
bigIntToArrayBuffer(B),
bigIntToArrayBuffer(S),
),
);
}
}

test("#SRPSession compatible with nimbusds java implementation, no U padding", async (t) => {
t.plan(3);

class TestRoutines extends SRPRoutines {
class TestRoutines extends NimbusRoutines {
public generatePrivateValue(): bigint {
return (
BigInt(
Expand Down Expand Up @@ -70,7 +99,7 @@ test("#SRPSession compatible with nimbusds java implementation, no U padding", a

test("#SRPSession compatible with java nimbus JS, U padding", async (t) => {
t.plan(1);
class TestClientRoutines extends SRPRoutines {
class TestClientRoutines extends NimbusRoutines {
public generatePrivateValue(): bigint {
return (
BigInt(
Expand All @@ -80,7 +109,7 @@ test("#SRPSession compatible with java nimbus JS, U padding", async (t) => {
}
}

class TestServerRoutines extends SRPRoutines {
class TestServerRoutines extends NimbusRoutines {
public generatePrivateValue(): bigint {
return (
BigInt(
Expand Down