JetBrains / teamcity-azure-active-directory

TeamCity plugin which supports authentication via Microsoft Azure Active Directory
Apache License 2.0
26 stars 19 forks source link

Critical: Signature is not verified #6

Closed erikbra closed 7 years ago

erikbra commented 9 years ago

In JWT.java, only the payload (claims) of the JWT token is read. This means that no verification of the signature is performed. In other words, anyone can modify the token and claim to be anyone.

Signature verification needs to be added.

jamescon commented 9 years ago

Are there any plans to fix this?

erikbra commented 9 years ago

@jamescon, I haven't heard anything since I reported the issue. @ekoshkin has not responded. I had jo quit using the plugin for this particular reason. I have filed another pull request on the project, #5, which fixes issue #3, but haven't heard anything on that one either. So I'm a little reluctant to fix this issue and file a pull request, if no one is going to respond to that one either.

It's a bit sad, because the functionality is a gem. Do you have any good writeups on how to verify the JWT signature against Azure AD, @jamescon? We need to get the public key in a backchannel, right?

sballarati commented 9 years ago

Hi, it seems that the Java JWT library can be used to fix this issue: https://github.com/jwtk/jjwt

Thanks, Sebastian

ghost commented 9 years ago

Hi @erikbra , @jamescon! AFAIK public key used to validate JWT signatures is global (for all AAD's running under all registered user accounts). So I can't clarify what is the additional value to implement signature checking in case we already use SSL connection to Azure login endpoint, Please correct me if I wrong.

erikbra commented 9 years ago

You must validate that the message has not been tampered with. You validate that the message has not been tampered with by validating the message signature.

Now I can pretend to be anyone in my organization by editing the JWT before posting it to Teamcity. (Using e.g. Fiddler). The signature is then invalid, but we don't notice.

  1. jun. 2015 15.37 skrev "Evgeniy Koshkin" notifications@github.com:

Hi @erikbra https://github.com/erikbra , @jamescon https://github.com/jamescon! AFAIK public key used to validate JWT signatures is global (for all AAD's running under all registered user accounts). So I can't clarify what is the additional value to implement signature checking in case we already use SSL connection to Azure login endpoint, Please correct me if I wrong.

— Reply to this email directly or view it on GitHub https://github.com/ekoshkin/teamcity-azure-active-directory/issues/6#issuecomment-111138123 .

erichexter commented 8 years ago

Sounds like a pull request.

erikbra commented 8 years ago

Sorry, I haven't been able to followup on this, I have looked briefly at the Jawa JWT library that @sballarati mentions. It looks like it can do the job. What are your feelings on bringing in an external library dependency, @ekoshkin? I would prefer using an actively maintained JWT library over taking in the portions we need. It makes maintaining the code easier.

I am a bit time challenged, but I might be able to help solve this if we can agree on how to solve it. This class (with the dependencies, of course), looks promising: https://github.com/jwtk/jjwt/blob/master/src/main/java/io/jsonwebtoken/impl/DefaultJwtParser.java

ghost commented 8 years ago

@erikbra using JWT library looks good to me

jfatta commented 7 years ago

@erikbra I submitted https://github.com/ekoshkin/teamcity-azure-active-directory/pull/20 to solve this. Thanks.