opensearch-project / security

🔐 Secure your cluster with TLS, numerous authentication backends, data masking, audit logging as well as role-based access control on indices, documents, and fields
https://opensearch.org/docs/latest/security-plugin/index/
Apache License 2.0
189 stars 271 forks source link

[BUG] SAML login fails when username contains '\h' due to JwtToken. #2531

Closed abhivka7 closed 1 month ago

abhivka7 commented 1 year ago

What is the bug? SAML login fails when username contains '\h'. it passes in all other cases such as username containing '\a' or '\x' . The error is coming from: (https://github.com/opensearch-project/security/blob/main/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java#319)

We tried logging in using two usernames :

  1. First username had a '\h' in it. We got a 500 internal error with this.
  2. Second username had a '\a' in it. We were able to login using this.

https://github.com/opensearch-project/security/blob/main/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java#L297 extracts similar jwtSubjectKey for usernames with '\h' and '\a'.

But the encoded JWT at : https://github.com/opensearch-project/security/blob/main/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java#L319

return different types of values for sub. For username with '\h', it returns (in encoded form) :

{2 items
"header":{1 item
"alg":"HS512"
}
"payload":{5 items
"nbf":1678363196
"exp":1678366796
"sub":"abcdhhabcd"
"saml_nif":"email"
"saml_si":"318941483"
}
}

For username with '\a', it returns (in encoded form) :

{2 items
"header":{1 item
"alg":"HS512"
}
"payload":{5 items
"nbf":1678363358
"exp":1678366958
"sub":"abcd\ahabcd"
"saml_nif":"email"
"saml_si":"693676161"
}
}

It is still unknown for how many characters will the issue repeat. How can one reproduce the bug? Steps to reproduce the behavior:

  1. Create a SAML domain, and try logging in using a username with '\h' and you will see 500 internal error.
  2. Now repeat same thing with '\a' or '\x'. This should work.

What is the expected behavior? User should be able to login even when username contains '\h'.

What is your host/environment?

Do you have any screenshots? If applicable, add screenshots to help explain your problem.

Do you have any additional context? Add any other context about the problem.

abhivka7 commented 1 year ago

I verified by logging (jwt.getClaims()).getSubject() ) just before jwtProducer.processJwt at https://github.com/opensearch-project/security/blob/main/src/main/java/com/amazon/dlic/auth/http/saml/AuthTokenProcessorHandler.java#L318.

It is giving the correct expected value of username with '\h'. So the issue is with the encodedJwt:

String encodedJwt = this.jwtProducer.processJwt(jwt);

peternied commented 1 year ago

@abhivka7 Thanks for filing and digging in, how would you feel about making a PR to fix this issue?

nancyjlau commented 11 months ago

Currently working on this issue. It seems like String encodedJwt = this.jwtProducer.processJwt(jwt); fails to generate a valid token when the input contains \h, and needs two backslashes \\h in order to work properly to save as one backslash in the JWT structure. Also, there is no way to have an odd number of backslashes (like 1, 3, 5, 7 backslashes), as I have tried it manually on the JWT Web Token site from Okta.

Here, you can see that for \h, it is not allowed to have odd numbers of backslashes:

Trying to input the username\hexample with 1 backslash as the saved "sub" (subject) value: Imgur (It is red and quite literally will not create a JWT token)

Trying to input the username\\\hexample with 3 backslashes as the saved "sub" (subject) value: Imgur (It is red and will not create a JWT token)

Trying to input the username\\\\\hexample with 5 backslashes as the saved "sub" value: Imgur (It is also red and refuses to create a JWT token)

A working JWT token with a backslash before h can only be created with an even number of backslashes beginning with \\h, which will be saved as \\h.

Then, there is terribly inconsistent backslash handling after this. Examples:

This is most likely due to the way how backslashes are treated as escape characters in string literals.

Since there have been cases where \h is requested to be part of a username, I will be submitting a PR today for \\h, which can be used as an alternative to \h, which cannot be used to generated a JWT token. I would recommend disallowing any more backslashes after two.

cwperks commented 11 months ago

@nancyjlau What is the full error message that String encodedJwt = this.jwtProducer.processJwt(jwt); produces when the subject has \h in it?

Does jax-rs have a general way to escape token claims? Is this only an issue with the sub claim or does this exception get thrown regardless of the claim.

FYI there is a PR to switch the JWT library used in this repo? Do we know if the new library has similar behavior?

Relevant PR: https://github.com/opensearch-project/security/pull/3421

MaciejMierzwa commented 11 months ago

JWT is Base64Url encoded and Bases64URL doesn't accept backslash character, although most of libraries don't enforce this rule: https://datatracker.ietf.org/doc/html/rfc4648#section-5 If escaped backslash is used, it seems like both cxf and nimbus jwt libraries work with it `

    //nimbus jwt
    JWSHeader header= new JWSHeader(JWSAlgorithm.HS512);
    JWTClaimsSet jwtClaimsSet = new JWTClaimsSet.Builder()
            .claim("2", "\\h")
            .claim("4", "\\\\h")
            .claim("6", "\\\\\\h")
            .claim("8", "\\\\\\\\h")
            .build();

    JWSSigner signer = new MACSigner("someSecretKeyThatsVeryHardToBreakAndHasAtLeast512BitsOrMoreToSupportHMAC512");
    SignedJWT jwt = new SignedJWT(header, jwtClaimsSet);
    jwt.sign(signer);
    jwt.serialize();
}

` This code would produce jwt: eyJhbGciOiJIUzUxMiJ9.eyI4IjoiXFxcXFxcXFxoIiwiMiI6IlxcaCIsIjQiOiJcXFxcaCIsIjYiOiJcXFxcXFxoIn0.T6KZHLgFEU1gnEXUhW0kc44PrpJB_TjDZuM62M0eAwaiFGX_K6sfr06SkcdB38wNuoAEziTBarQfdVaHQpvAiA which seems to get parsed ok.

image Regarding characters \t \n \x \a \h, I think java will try to either escape backslash during deserialization/serialization, or maybe try to treat it as special characters (tab, new-line etc).

martin-gaievski commented 4 months ago

@peternied @cwperks folks is it possible to get more traction for this issue? We do have more requests referring to the same wrong behavior.

peternied commented 4 months ago

@martin-gaievski could you make a contribution for this issue?

martin-gaievski commented 4 months ago

will take a look at my free time, I was looking for support from maintainers/active contributors similar to how that's done in other Opensearch plugin repos.

cwperks commented 4 months ago

@martin-gaievski I think I finally got to the bottom of this. This is an issue with the cxf library that the security plugin uses to issue JWTs for SAML auth.

CXF thinks that \h is a special character that needs to be escaped: https://github.com/apache/cxf/blob/main/rt/rs/extensions/json-basic/src/main/java/org/apache/cxf/jaxrs/json/basic/JsonMapObjectReaderWriter.java#L52

PR where it was introduced: https://github.com/apache/cxf/pull/819/files#diff-3396d540d530ac37f53c476b87ef6fdb86343aa6ff211826418438171e05c764

I can't find an official list of characters that need to be escaped in JSON, but https://stackoverflow.com/a/27516892 seems to indicate that it isn't one. I will open up a PR in cxf to solicit comments.

@reta Fancy seeing you in that codebase! Do you know if h can be removed from the ESCAPED_CHARS set or if there is documentation about what the \h character is?

cwperks commented 4 months ago

Opened a PR in cxf: https://github.com/apache/cxf/pull/1872

I was able to validate that removing h from ESCAPED_CHARS in cxf resolves this issue.

To test I:

I've attached the docker configuration below. The user list is configured in saml-demo/config/authsources.php and contains a user user4\hexample:user4pass. Before the change in CXF it fails to login with internal server error and after the change the user is able to login.

2.11.0-saml-plus-basic-multiauth 2.zip

reta commented 4 months ago

@reta Fancy seeing you in that codebase! Do you know if h can be removed ESCAPED_CHARS set or if there is documentation about what the \h character is?

:-D @cwperks I think this is a typo, h -> n, as per https://datatracker.ietf.org/doc/html/rfc8259#section-7:

 string = quotation-mark *char quotation-mark      char = unescaped /
          escape (
              %x22 /          ; "    quotation mark  U+0022
              %x5C /          ; \    reverse solidus U+005C
              %x2F /          ; /    solidus         U+002F
              %x62 /          ; b    backspace       U+0008
              %x66 /          ; f    form feed       U+000C
              %x6E /          ; n    line feed       U+000A
              %x72 /          ; r    carriage return U+000D
              %x74 /          ; t    tab             U+0009
              %x75 4HEXDIG )  ; uXXXX                U+XXXX      
escape = %x5C              ; \      
quotation-mark = %x22      ;
unescaped = %x20-21 / %x23-5B / %x5D-10FFFF 

Could you open an issue here https://issues.apache.org/jira/projects/CXF/issues? (if not, I could open it on your behalf)

cwperks commented 4 months ago

I don't think I have permissions to create an issue. I'll go through the self serve signup page to request an account.

cwperks commented 4 months ago

@reta Created an issue: https://issues.apache.org/jira/browse/CXF-9015

varun-lodaya commented 3 months ago

Nice! Thanks @cwperks and @reta. @cwperks seems your fix is merged, can we track this on OpenSearch for next release (2.15)?

reta commented 3 months ago

Nice! Thanks @cwperks and @reta. @cwperks seems your fix is merged, can we track this on OpenSearch for next release (2.15)?

Sure, we would need to have Apache CXF released first, I am not sure it will happen before 2.15.0

reta commented 1 month ago

@cwperks new releases just landed [1], we should be able to fix this issue in time for 2.16.0, thanks!

[1] https://cxf.apache.org/download.html

cwperks commented 1 month ago

@reta Thanks for following up! I will raise PRs on the release branches.

DarshitChanpura commented 1 month ago

@cwperks Can this issue be closed now?

cwperks commented 1 month ago

Yes it can be. Closing.