[Java] CWE-347: Query for detecting Signature Exclusion Attack with SAML assertion

luchua-bc commented 3 years ago

Query

Link to pull request with your CodeQL query:

Relevant PR: https://github.com/github/codeql/pull/6935

CVE ID(s)

List the CVE ID(s) associated with this vulnerability. GitHub will automatically link CVE IDs to the GitHub Advisory Database.

CVE-2020-12676

Report

Describe the vulnerability. Provide any information you think will help GitHub assess the impact your query has on the open source community.

SAML (Security Assertion Markup Language) is an XML-based markup language for exchanging authentication and authorization data between parties, in particular, between an identity provider and a service provider. It is used for security assertions, which are statements that service providers use to make access-control decisions. SAML response messages consist of issuer, assertion (subject, conditions, and statements), signing key, and signature.

If SAML token does not contain any signature, no protection of integrity or authenticity is provided. Since no digital signature for the token is required, an attacker can generate tokens containing arbitrary identities of other users.

Java applications use the JAXB (Java Architecture for XML Binding) API for unmarshalling (reading) XML instance documents into Java content trees, and then marshalling (writing) Java content trees back into XML instance documents. The Java XML Digital Signature APIs are specified in JSR-105 and are implemented in the javax.xml.crypto package, which are used to verify signature of SAML XML documents.

The query detects missing signature check against SAML assertion XML documents.

[x] Are you planning to discuss this vulnerability submission publicly? (Blog Post, social networks, etc). We would love to have you spread the word about the good work you are doing

Result(s)

Provide at least one useful result found by your query, on some revision of a real project.

Fixed SAMLv2 signature check with fusionauth-samlv2

ghsecuritylab commented 3 years ago

Your submission is now in status Generate Query Results.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

ghsecuritylab commented 3 years ago

Your submission is now in status FP Check.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

pwntester commented 3 years ago

Hi @luchua-bc

Thanks for the submission! The query returned 0 results and therefore the "scope" does not meet the minimal value to qualify for a bounty. Can you please upload the vulnerable version of FusionAuth to LGTM so we can consider that one as a valid result?

Thanks

luchua-bc commented 3 years ago

Hi @pwntester,

Thanks for reviewing this PR. I think no database was created on your end therefore no valid result can be returned.

The issue was caused by that the project FusionAuth uses a special build tool named Savant while the CodeQL tool only recognizes several common build tools including maven, ant, and gradle. To address this issue, we can add a pom.xml to its root directory so that the project can be built with maven. Note as GitHub doesn't allow xml attachment, I've changed the extension to txt, you can remove it if you want to give it a try. pom.xml.txt

And here is the database generated with this pom.xml saml.zip.

Please validate. Thanks.

@luchua-bc

pwntester commented 3 years ago

@luchua-bc can you please upload that project to LGTM under your own github user?

luchua-bc commented 3 years ago

Hi @pwntester,

As LGTM doesn't take cloned project, I've copied the project under my own github user and added a note to its README and LICENSE files specifying the project is for CodeQL research purpose.

After the project was uploaded to LGTM, I created a query. Executing the query gives the following valid result:

Please validate. Thanks.

pwntester commented 3 years ago

Thanks

ghsecuritylab commented 3 years ago

Your submission is now in status CodeQL review.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

ghsecuritylab commented 3 years ago

Your submission is now in status SecLab finalize.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

pwntester commented 3 years ago

Hi @luchua-bc,

Thanks for the submission! We have reviewed your report and validated your findings. After internally assessing the findings and the query we have determined this query doesn't find any other hit on the lgtm.com project corpus, so it's not general-purpose enough to go into the repository. Therefore the query is not eligible for a reward under the Bug Bounty program.

Best regards and happy hacking!

ghsecuritylab commented 3 years ago

Your submission is now in status Closed.

For information, the evaluation workflow is the following: SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

luchua-bc commented 3 years ago

Hi @pwntester,

Signature exclusion attack is indeed a valid type of general-purpose attacks. If you google "signature exclusion attack github", you can find many hits.

The well-maintained and widely used HackTricks website includes it. And the Burp pentesting plugin checks it as well.

SAML integration is mainly used for enterprise applications so this vulnerability cannot be as common as other simple issues like unencrypted passwords, however, it's indeed a general-purpose attack. And IMHO Security Lab shall include more queries for enterprise applications that may not be in sample apps/test apps in public repositories.

@luchua-bc

pwntester commented 3 years ago

Hi @luchua-bc

We are aware that signature exclusion attacks are general purpose attacks and I have actually found novel vectors to related with them in the past (https://github.com/pwntester/DupeKeyInjector) but your query just finds a particular case of Signature exclusion attacks and therefore the scope rating was low.

I agree that OSS software may not use SAML as much as enterprise code but it is also true that enterprise code does not normally implement SAML token parsing code themselves, and instead use existing libraries for that (many of them are OSS).

luchua-bc commented 3 years ago

Thanks @pwntester for the reply.

My query handles two new cases - skip the check when signature is missing, which is the case of the CVE referenced, and no SAML signature check at all, which are all cases of signature exclusion attack I can think about. Your DupeKeyInjector handles some cases of XML signature wrapping attacks, which are of a different type, if I understand correctly.

And after I submitted the query, the submission passed the "Generate Query Results" phase when additional requested information was gathered and provided. Also some Security Lab staff spent the time and efforts to help update and finalize this query. IMHO efforts could have been saved if the decision had been made earlier. I'm not happy with the fact that the query was finalized then rejected, which never happened before and is to my big surprise but the Security Lab have the final ruling of the submission.

pwntester commented 3 years ago

Hi @luchua-bc

Your DupeKeyInjector handles some cases of XML signature wrapping attacks, which are of a different type, if I understand correctly.

Its a key confusion attack rather than a wrapping attack, but to clear it up, I was not asking you to support that particular case which was also a very specific case we found affecting .NET libs

And after I submitted the query, the submission passed the "Generate Query Results" phase when additional requested information was gathered and provided. Also some Security Lab staff spent the time and efforts to help update and finalize this query. IMHO efforts could have been saved if the decision had been made earlier.

I think we may not have defined the triage phases very clearly and that has probably caused some confusion:

SecLab review is a shallow and light review to filter non-security or non-relevant queries as early as possible in the process so no resources from the CodeQL or SecLab teams are wasted on queries that we know upfront, are going to be rejected because they don't meet the basic criteria. In this phase the SecLab team assigns an initial Scope and Impact ratings based on query coverage and CWE related CVSS scores. These ratings are subject to change in the following phases. And indeed the Scope one is very sensitive to the LGTM results as it was in your case.
Generate Query Results is led by the CodeQL team which will prepare and run the query on an extensive LGTM projects corpus. The CodeQL team might request query changes to the authors if they feel there is something that can improve the LGTM results.
FP checks When the results are ready, the SecLab will review them and assess the FP rating and normally modify the Scope rating. After having all the ratings scored, the SecLab discusses with the CodeQL team if the query is worth adding to the main or experimental query sets and a decision to reject the query may happen at this time. This is what happened with your query.
Code Review This is when the CodeQL team performs the in-depth query review that normally involves making it performant and accurate to be included either on the experimental or main sets. As a result the query documentation and maturity are also rated
SecLab finalize will decide on a final bounty level taking the previous SecLab ratings but also new CodeQL ratings from the CodeQL team
Pay is when the final HackerOne payment is done.

I'm not happy with the fact that the query was finalized then rejected, which never happened before and is to my big surprise but the Security Lab have the final ruling of the submission.

I think the confusion arises because your query was moved into CodeQL review, but note that this does not imply that the query is accepted since it still requires additional ratings from the CodeQL team. The conversation between the CodeQL and SecLab teams normally occurs at the end of the FP checks, but in this case it happened at the beginning of the CodeQL review. However it was decided to reject the query before any further review was done.

We will try to improve our process so that this conversation always happens in the FP checks phase and reduce confusion.

As you can see, from the phases description above, your query was not finalized since it was not reviewed by the CodeQL team.

We apologise if the changes requested in the SecLab review or Generate CodeQL results caused more work for you, but their only purpose is to get your query in the best form to face the FP checks phase.

luchua-bc commented 3 years ago

Thanks @pwntester for the detailed explanation of the Security Lab process and it helps to clarify the flow.

However, the decision making process is still not clear to me. For the "All for one, one for all (add a new default query)" category of the Security Lab policy published for the bug bounty program with HackerOne, it only requires:

a CodeQL query that models a vulnerability class not currently covered by the current queries
submission with at least one result of CVE
include at least one useful result on some revision of a real project

Although it also mentions "We will assess if the query meets the standards to be included in the CodeQL repo, or whether improvements are required.", my understanding is that the standard is the three criteria stated above judging from its context.

I noticed no more than 1 in 4 public GitHub projects can generate databases for CodeQL queries without any modification due to factors such as obsolete build scripts, missing build scripts, layout of the project file directories, or unsupported build scripts like the one used in the referenced CVE. Therefore I think the conclusion "this query doesn't find any other hit on the lgtm.com project corpus, so it's not general-purpose enough to go into the repository." is pretty subjective and later on we clarified signature exclusion attack is indeed a general-purpose attack.

Making the triage process objective will help the community to contribute to this project. I would recommend to make the policy more clear, or require at least two results on two real projects, or add a statement saying Security Lab has an internal review process, which has the final ruling and may consider non-published factors, in addition to published criteria.

pwntester commented 3 years ago

Thanks for the feedback @luchua-bc we will take it into account to rephrase the program conditions and make them as clear and objective as possible.

luchua-bc commented 3 years ago

Thanks @pwntester for the follow-up. Rephrasing the program conditions to clarify the ruling criteria will be helpful for future submissions by the community although existing queries shall be triaged with the currently published criteria to make the process fair and consistent IMHO. For example, the current criteria require at least one CVE, which was not a requirement when the program just launched, therefore submissions before the criteria change shall still be triaged following the old process that didn't require a CVE.

pwntester commented 3 years ago

Thanks again for the feedback @luchua-bc its highly welcomed.

The currently published criteria states that the amount for the bounty is determined by:

The complexity of the vulnerabilities
The severity of the vulnerabilities
The prevalence of the vulnerabilities: the number of impacted users and systems
The performance and reliability of the query: its false positive rate
The documentation of the query
Whether you produce a blog post / write-up about the vulnerabilities and query to help share your experience

These are independent coordinates used to determine the final bounty. Not modelling a CVE makes a query non-applicable, However, if the query models a CVE but it fails to return any results in our test corpus (which contains ~30k projects for Java) or other projects provided by the submitter, then the value of the Prevalance rating drops to 0 and makes it impossible to evaluate the Reliability rating. Those values makes the final bounty to fall into the lowest value (0$).

In addition, the current program states that:

Queries must also be sufficiently general to cover a class of vulnerabilities and may not be tailored to a specific CVE.

Even though your queries can find specific signature exclusion attacks when using the javax.xml.crypto.dsig package, we consider it to be too tailored to a specific CVE.

We really appreciate this and past contributions and encourage you to keep submitting great queries!

github / securitylab