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

[📜] Spec for declaration of vulns to help scanners check find rate #1441

Closed
bkimminich opened this issue Aug 10, 2020 · 36 comments
Closed
Labels

Comments

@bkimminich
Copy link
Member

Back in 2016 an idea of having a __vulns.json file in vulnerable applications came up and was prepared by members of the OWASP ZAP, VWAD and Juice Shop teams. It was supposed to allow scanners/tools to assess their success rate by

  1. iterating over the vulnerabilities array
  2. looking for one or more matches from each flags array in their own reported finding descriptions/root causes etc.

The Juice Shop version of this file looked like this back then:

{
  "application" : "juice-shop",
  "vulnerabilities" : [
    {"type" : "xss", "flags" : ["/#/search", "q="] },
    {"type" : "sqli", "flags" : ["/#/search", "q="] },
    {"type" : "sqli", "flags" : ["/#/login", "email"] },
    {"type" : "sqli", "flags" : ["/#/login", "password"] },
    {"type" : "crypto", "flags" : ["/ftp/eastere\\.gg", "base64", "L2d1ci9xcmlmL25lci9mYi9zaGFhbC9ndXJsL3V2cS9uYS9ybmZncmUvcnR0L2p2Z3V2YS9ndXIvcm5mZ3JlL3J0dA=="] },
    {"type" : "access", "flags" : ["/#/administration"] },
    {"type" : "access", "flags" : ["/#/score-board"] },
    {"type" : "hash", "flags" : ["/api/Users", "md5"] },
    {"type" : "hash", "flags" : ["/api/Users/[0-9]*", "md5"] },
    {"type" : "access", "flags" : ["/api/Feedbacks/[0-9]*", "delete"] },
    {"type" : "access", "flags" : ["/api/BasketItems/[0-9]*", "put", "quantity"] },
    {"type" : "session", "flags" : ["sessionStorage", "bid" ] },
    {"type" : "csrf", "flags" : ["/rest/user/change-password", "get" ] },
    {"type" : "trustbound", "flags" : ["/rest/user/change-password.*^((?!current).)*$", "new", "repeat" ] },
    {"type" : "trustbound", "flags" : ["/#/complain", "/file-upload", "size" ] },
    {"type" : "trustbound", "flags" : ["/#/complain", "/file-upload", "type" ] },
    {"type" : "session", "flags" : ["/#/contact", "userId", "hide" ] },
    {"type" : "xss", "flags" : ["/#/contact", "comment" ] },
    {"type" : "xss", "flags" : ["/#/contact", "comment" ] },
    {"type" : "crypto", "flags" : ["/#/basket", "coupon", "(z85|base85)" ] },
    {"type" : "access", "flags" : ["/the/devs/are/so/funny/they/hid/an/easter/egg/within/the/easter/egg" ] },
    {"type" : "access", "flags" : ["css/geo-bootstrap/swatch/bootstrap\\.css" ] },
    {"type" : "access", "flags" : ["/i18n/tlh\\.json" ] },
    {"type" : "access", "flags" : ["/ftp/.*%2500\\.md" ] },
    {"type" : "access", "flags" : ["/ftp/.*%2500\\.pdf" ] },
    {"type" : "access", "flags" : ["/ftp/.*?md_debug=.*\\.pdf" ] },
    {"type" : "access", "flags" : ["/ftp/.*?md_debug=.*\\.md" ] },
    {"type" : "redirect", "flags" : ["/redirect?to=.*?.*=https://github.com/bkimminich/juice-shop" ] },
    {"type" : "redirect", "flags" : ["/redirect?to=.*?.*=https://blockchain.info/address/1FXJq5yVANLzR6ZWfqPKhJU3zWT3apnxmN" ] },
    {"type" : "redirect", "flags" : ["/redirect?to=.*?.*=https://gratipay.com/juice-shop" ] },
    {"type" : "redirect", "flags" : ["/redirect?to=.*?.*=http://flattr.com/thing/3856930/bkimminichjuice-shop-on-GitHub" ] },
    {"type" : "xss", "flags" : ["/#/administration", "post", "/api/Users", "email" ] },
    {"type" : "xss", "flags" : ["/#/search", "post", "/api/Products", "description" ] },
    {"type" : "xss", "flags" : ["/#/search", "put", "/api/Products/[0-9]*", "description" ] },
    {"type" : "crypto", "flags" : ["/#/score-board", "continueCode", "hashid" ] }
  ]
}

The JSON spec was the following (see https://github.com/OWASP/OWASP-VWAD/blob/6a8b54004db5b48278e82f2cc856fdc229c80ca3/src/owasp-wiki/deprecated/vulns-json/schema.json)

{
    "$schema": "http://json-schema.org/draft-04/schema#",
    "title": "Vulnerability matchers for an intentionally vulnerable web application",
    "type": "object",
    "properties": {
        "application": { "description": "The unique name of the vulnerable web application", "type": "string" },
        "vulnerabilities": {
                "type": "array",
                "items": {
                    "type": "object",
                    "properties": {
                        "type" : { "enum": [ "xss", "sqli", "crypto", "access", "hash", "session", "csrf", "trustbound", "redirect", "leakage" ] },
                        "flags": {
                          "type": "array",
                          "items": {
                              "type": "string"
                          },
                          "minItems": 1,
                          "uniqueItems": true
                        }
                    },
                    "required": ["type", "flags"]
                },
                "uniqueItems": true
            }
    },
    "required": ["application", "vulnerabilities"]
}

Because the idea was not pursued further and it saw no adoption, it was dropped in 2017.

Now, 3 years later, let's try that again, shall we?

In this issue ideas can be collected and the original schema can be taken apart. Personally, I still think a light-weight approach is a good idea, but some meta data could be added (like URL of the application repository as a unique identifier) or existing one improves (like the - in hindsight - seemingly arbitrary enumeration of type entries) - Any constructive criticism is welcome.

To express if you think this idea is good (👍) or worthless (👎), please use the reactions on this issue instead of posting +1/-1 comments. Thanks!

@bkimminich bkimminich changed the title [📜] 2nd attempt on a __vulns.json spec to help scanners measure success vs. vulnerable apps [📜] Spec for declarative file to help scanners measure success vs. vulnerable apps Aug 10, 2020
@bkimminich bkimminich changed the title [📜] Spec for declarative file to help scanners measure success vs. vulnerable apps [📜] Spec for declaration of indented vulns to help scanners check find rate Aug 10, 2020
@bkimminich bkimminich changed the title [📜] Spec for declaration of indented vulns to help scanners check find rate [📜] Spec for declaration of vulns to help scanners check find rate Aug 10, 2020
@preetkaran20
Copy link

Hi @bkimminich ,

Adding https://owasp.slack.com/archives/CPMEWT342/p1597085979033700 and SasanLabs/VulnerableApp#164 for reference.

thanks,
Karan

@bkimminich
Copy link
Member Author

.vulns.yml (Draft 1)

Conceptually same as the original just in YAML and with source location and identifiers instead of arbitrary list of vuln types:

application:
  name: OWASP Juice Shop
  source: https://github.com/bkimminich/juice-shop
vulnerabilities:
  -
    type:
      - "CWE-79"
      - "A7:2017"
      - "WASC-8"
    flags:
      - "/#/administration"
      - "POST"
      - "/api/Users"
      - "email"
  -
    type:
      - "CWE-74"
      - "A1:2017"
      - "API8:2019"
      - "WASC-19"
    flags:
      - "/rest/search"
      - "q="

@preetkaran20
Copy link

preetkaran20 commented Aug 11, 2020

Hi @bkimminich ,

Following are my queries:

  1. Can we have separate identifier for vulnerability types like:
type:
      CWE:
          - "CWE-79"
       WASC:
          - "WASC-8"

such that anyone that is parsing knows what are the identifier types so that they need not to break values and then know what the identifier is.

  1. I am not getting what does the flags mean here and why these are list of flags ?

Also This is quite different from my thinking where in i have written in a way that each URL has Vulnerabilities linked so that scanner Alert for that URL can be matched with the Vulnerabilities linked something like:
example vuln.json:

url:
     - "/vuln/xss/level1"
type: 
      - CWE:
          - "CWE-79"
        WASC:
          - "WASC-8"
      - CWE:
          - "CWE-89"
        WASC:
          - "WASC-5"

ZAP alerts:

Url:
     - "/vuln/xss/level1"
PluginId:
    - "6"
VulnerabilityType:
  CWE:
    - "CWE-79"

now URL is the key which can be used to map the findings.

thanks,
Karan

@bkimminich
Copy link
Member Author

Flags are indicators that might show up in a scanner report. The URL that has an XSS problem can be a flag. The parameter name susceptible to the payload could be a second one. The responsible code snippet might be a third. Etc.

The more flags the scanner report contains, the more likely it found the actual vulnerability. Even the CWE, WASC or OWASP ids could be just more flags, as at least one of those probably shows up in the scanner finding as a reference. I'd put those separate still to avoid confusion and make it a bit human readable too. But I don't think named properties per id issuer are really needed.

@bkimminich
Copy link
Member Author

Your ZAP alert example is perfect, it would match the URL flag and the CWE type in its finding, so it's very likely a true positive.

If it would report the URL but some different CWE, it maybe missed or misclassified the actual vuln.

@preetkaran20
Copy link

Hi @bkimminich ,

Yes this makes sense and i think we can divide '/scanner' endpoint in SasanLabs/VulnerableApp into 2 parts one is the way to help scanners find the vulnerability (its requirements) and other is something similar to __vuln.json.

Also one doubt i have related to scanners is, are we thinking that scanner report/alerts will have all these flag information ? like say we have a flag and say scanners are not having that information in alerts then that can be little tricky or are we assuming that scanners will save the entire response in the report ?

thanks,
Karan

@J12934
Copy link
Member

J12934 commented Aug 11, 2020

What I don't quite get is how telling a Scanner how a vulnerability can be discovered is helpful. It sounds to me as this gives the tools more information than it could ever have about a real app.

I think the hints shouldn't go further than something like an OpenAPI definition of the API, to ensure that the scanner is able to assess all endpoints to its fullest potential. More than that feels like "cheating".

Having hint to help the tools assess how many vulnerabilities it has discovered seems more helpful, thought this can normally be determined in a couple of minutes when comparing the findings against the vulnerabilities of the vuln app. Would be interesting how if this could help this process.

@bkimminich
Copy link
Member Author

bkimminich commented Aug 11, 2020

Also one doubt i have related to scanners is, are we thinking that scanner report/alerts will have all these flag information? like say we have a flag and say scanners are not having that information in alerts then that can be little tricky or are we assuming that scanners will save the entire response in the report ?

No, that's why we might want to have rather smaller fragmented flags instead of long strings which might not be exact matches. Another option would be to support RegEx-flags optionalle or make flags RegExes by default. Here is a hopefully helpful example of how I imagine it to work:

For the XSS vulnerability of the search box in Juice Shop the __vulns.json definition might look like this, assuming all flags are RegExes:

application:
  name: OWASP Juice Shop
  source: https://github.com/bkimminich/juice-shop
vulnerabilities:
  -
    type:
      - "CWE-79"
      - "CWE-749"
      - "A7:2017"
      - "WASC-8"
    flags:
      - "DomSanitizer"
      - "bypassSecurityTrustHtml"
      - "q="
      - "#/search"
      - "main-es[0-9]*.js"

Here's a finding from a ZAP baseline scan report that actually found the usage of an insecure JavaScript function:

image

My suggestion for the scanner provider would now be to run all the flags and type each over their report to look for matches. In this example we would get the following outcome for OWASP ZAP:

  • types
    • CWE-79 = ❌
    • CWE-749 = ✔️
    • A7:2017 = ❌
    • WASC-8 = ❌
  • flags
    • DomSanitizer = ❌
    • bypassSecurityTrustHtml = ✔️
    • q= = ❌
    • #/search = ❌
    • main-es[0-9]*.js = ✔️x2

The interpretation of this result is up to the vendor of the scanner in my opinion, but in this case it's pretty clear that ZAP found the root cause for some potential XSS, but - at least in this finding - not its execution. In the actual report I used the execution was not found, but we could imagine ZAP to report another finding from the same scan that matches some of the execution flags. As a scanner vendor I'd then be happy that I found some real issue from the execution and root cause angle. Well done. Repeating this for all specified vulnerabilities would give you a good picture of how well the scanner did on OWASP Juice Shop.

Addendum: In the Juice Shop there are several XSS holes, and at least 3 or 4 of them come from using the DomSanitizer.bypassSecurityTrustHtml function. They would all get their own vuln spec, but they'd all share the flags DomSanitizer, bypassSecurityTrustHtml and main-es[0-9]*.js because that's the root cause for all of them. The other flags would be different, because the execution would happen in different locations. In the case of ZAP's report, all 3-4 XSS vulns would match the same flags on the finding seen in the screen shot above. That's not bad at all, even though ZAP missed all actual XSS executions.

@preetkaran20
Copy link

preetkaran20 commented Aug 12, 2020

What I don't quite get is how telling a Scanner how a vulnerability can be discovered is helpful. It sounds to me as this gives the tools more information than it could ever have about a real app.

I think the hints shouldn't go further than something like an OpenAPI definition of the API, to ensure that the scanner is able to assess all endpoints to its fullest potential. More than that feels like "cheating".

Actually '/scanner' endpoint point is like an openAPI definition only, it helps scanners know about the endpoint requirement like

{
    "url": "http://192.168.0.148:9090/vulnerable/JWTVulnerability/LEVEL_1",
    "location": "QUERY_PARAM",
    "parameter": "JWT",
    "sampleValues": [
      "ey.."
    ],
    "method": "GET",
    "vulnerabilityTypes": [
      "CLIENT_SIDE_VULNERABLE_JWT"
    ]
  }

Now this information is telling that "JWT" query param is the one where scanners need to inject the payload.

Scanners first find the endpoint (using spider) then they do the active scan (This is fully automated version where not manual intervention is there) but if they want to inject some payload they need to know where to provide the payload without that it is not possible for them to find the parameter which contains the payload.

Now this scanner endpoint helps them to know the requirements of the endpoint. I was thinking to have URL (as a flag) and single end point to serve both the purposes but now __vuln.json makes more sense.

Now the concern related to it is, in real applications this information is not there and how this information can be utiilsed ?
So my thinking is: in general while running a scanner we are proxying some of the endpoints and scanner saves the requests and hence when active scanner runs it utilises the same request to inject at some places and trying to find the vulnerability but this manual request proxying is not there if we run the fully automated scan so i was thinking of having following steps:

Passive scan (spider)
Read `/scanner` api and fire some requests (Kind of replication of manual request proxying step)
Run Active Scanner
Read __vuln.json compare the results and give the report

Going further may be scanner only understand this '/scanner' endpoint or openApi endpoint and incorporate it into the scan rule framework.

Thanks,
Karan

@bkimminich
Copy link
Member Author

bkimminich commented Aug 12, 2020

My own confusion with the /scanner endpoint is the actual use case. If it's in vuln apps, all we do is "help" the scanners do their job but it's highly unlikely any real world application will ever do that. Because adding such an endpoint to your app means you really put some thought and effort into your app security - but those are not the usual targets of scanners, right?

But your proposed steps clearly separate the two concepts. That's probably the most important thing, to allow clear expectation management when choosing one or both the concepts for your own (vuln) app.

@preetkaran20
Copy link

I think we can have a call and discuss on this. I need to go through your previous comment too. Let me think more and then if possible we can have a call to discuss.

@preetkaran20
Copy link

Hi @bkimminich ,

Thanks for the amazing explanation. Following are my thoughts:
we might need to make the vuln.json to contains Mandatory and addition parameters for more granular findings like in your example main-es[0-9]* is something mandatory and other parameters are for reaching specific search box. e.g.

vulnerabilities:
  -
    type:
      - CWE :
        - 79
      - A7:
        - 2017
      - WASC:
         - 8
    flags:
       - Mandatory:
         - "main-es[0-9]*.js"
      - Additional:
           - "#/search"
           - "DomSanitizer"
           - "bypassSecurityTrustHtml"
           - "q="

I also think at the same point that mandatory is something like URL (may be always) and everything else can go the same way you have explained.

Also we can divide flags further into Regex one's and non-Regex one's, this separation can be either as:

 flags:
       - Mandatory:
         - Regex:
              - "main-es[0-9]*.js"
         - Non-Regex:
             - "#/search"
      - Additional:
           - Regex:
           - Non-Regex:
               - "DomSanitizer"
               - "bypassSecurityTrustHtml"
               - "q="

or we can have some other way to differentiate it, However i also think that we can even go with regex always.

thanks,
Karan

@bkimminich
Copy link
Member Author

Having mandatory flags would be too limiting or at least put too much anticipation into the whole thing. A scanner might explore the search field and get an XSS executed, another one - like ZAP - might find the insecure function call in the minified JS, yet another might find something in the source code (I didn't add flags for that yet) etc.

@preetkaran20
Copy link

preetkaran20 commented Aug 12, 2020

But URL might remain the same and that can be mandatory and in case of minified versions we need not to qualify the parameter as mandatory because it changes at runtime.
If we think about server side vulnerabilities like SQLInjection, it can be very important like say 2 levels are there sqli/level1?q=<something> and sqli/level2?q=<something> even if scanner has not found the vulnerability in second endpoint still it can claim that it has found the vulnerability (just a way to reduce the false positives).

@bkimminich
Copy link
Member Author

I get your point, but if you are a SAST scanner you won't know about any URLs. You'd love some class names, function or variable names or a line of code reference etc.

@preetkaran20
Copy link

My own confusion with the /scanner endpoint is the actual use case. If it's in vuln apps, all we do is "help" the scanners do their job but it's highly unlikely any real world application will ever do that. Because adding such an endpoint to your app means you really put some thought and effort into your app security - but those are not the usual targets of scanners, right?

So the usecase which i was thinking is, say an endpoint is vulnerable to sqlInjection like sqli/level1?p=1 here p is vulnerable to sqlInjection and active scanner need to know about this parameter without that it can only guess the query params name. Now even if scanner is capable of finding the sqli but still it can't because this information is missing.

This can suit to our usecase where ZAP can configure the scan job and that job is capable to rate them self.

I think same can be applied to real world applications where it can help those applications to do some kind of basic security sanity using this approach.
However i also think that may be same can be achieved if they save the spidered information and use that.

Please correct me if i am thinking wrong here.

@bkimminich
Copy link
Member Author

bkimminich commented Aug 12, 2020

I would prefer real-world-app-developers to focus on not having Injection issues in their webapp instead of writing any declarations to help security tools find potential Injection issues. That's what you pay the security tool vendors for, to find the broken stuff. If they don't, they're not good at their job. Giving them training wheels won't make them better...😁

But giving them something to compare their unassisted (!) success rate against as a vulnapp-developer, that helps them work on their deficits... 😇

@preetkaran20
Copy link

preetkaran20 commented Aug 12, 2020

I get your point, but if you are a SAST scanner you won't know about any URLs. You'd love some class names, function or variable names or a line of code reference etc.

Ahh you are talking about SAST tools like Fortify or SonarQube etc ? I haven't thought about them. Thanks for mentioning them.
I think if we go with the assumption of handling both the types of analysers like Static and Dynamic using the same configuration then scanner might never achieve all the flags present in configuration and they might not even know what they need to match and not to match. How about we build it something like:

vulnerabilities:
  -
    scannerType:
    - "Dynamic scanning"
    type:
      - CWE :
        - 79
      - A7:
        - 2017
      - WASC:
         - 8
    flags:
       - Mandatory:
         - "main-es[0-9]*.js"
      - Additional:
           - "#/search"
           - "DomSanitizer"
           - "bypassSecurityTrustHtml"
           - "q="

 -
    scannerType:
    - "Static scanning"
    type:
      - CWE :
        - 79
      - A7:
        - 2017
      - WASC:
         - 8
    flags:
      - Additional:
           - "#/search"
           - "DomSanitizer"
           - "bypassSecurityTrustHtml"
           - "q="

@bkimminich
Copy link
Member Author

As a vulnapp developer, I don't really want to do the exercise of assuming what a DAST and what a SAST scanner will find. I can't imagine many other will... I also don't want to prescribe how many flags matches are needed to be considered a "good job" vs. "bad job" of the scanner. That's up to the scanner vendors in my opinion.

@preetkaran20
Copy link

As a vulnapp developer, I don't really want to do the exercise of assuming what a DAST and what a SAST scanner will find. I can't imagine many other will... I also don't want to prescribe how many flags matches are needed to be considered a "good job" vs. "bad job" of the scanner. That's up to the scanner vendors in my opinion.

Hmm makes sense. 👍

@preetkaran20
Copy link

I would prefer real-world-app-developers to focus on not having Injection issues in their webapp instead of writing any declarations to help security tools find potential Injection issues. That's what you pay the security tool vendors for, to find the broken stuff. If they don't, they're not good at their job. Giving them training wheels won't make them better...😁

But giving them something to compare their unassisted (!) success rate against as a vulnapp-developer, that helps them work on their deficits... 😇

Haha, actually developers are not writing it, if we look at Swagger etc they are doing similar job only (giving a schema to clients to know how to call) similarly my attempt was to give the same information from vulnerableApp. Ok i will rethink on this. 👍

@psiinon
Copy link

psiinon commented Aug 13, 2020

cc @sj @greysteil who may well have some SAST related comments

@preetkaran20
Copy link

As a vulnapp developer, I don't really want to do the exercise of assuming what a DAST and what a SAST scanner will find. I can't imagine many other will... I also don't want to prescribe how many flags matches are needed to be considered a "good job" vs. "bad job" of the scanner. That's up to the scanner vendors in my opinion.

Hi @bkimminich ,

I was thinking more about it and was thinking that if we are not going to categorise these flag then it might make Scanner's job much more tough reason being the one who is writing the vulnerability knows more about the flags like "#search" is an identifier to search box than the one who is understanding vuln.json.

And coming to the point of DAST and SAST, i think we just need to categorise between one who is accessing the Source Code and one who is doing a black box scanning like ZAP (Where JS/Html can be an exception to this)

thanks,
Karan

@bkimminich
Copy link
Member Author

bkimminich commented Aug 15, 2020

I agree that putting more time into a more explicit vuln specification makes the life of the scanner vendor a lot easier. But it makes the like of the vuln app developer a lot harder, because now they have to come up with the mapping between flags more likely to be found by SAST vs. DAST tools etc.

With the simple "here's a set of flags, good luck!" approach the interpretation becomes entirely the scanner vendor's job. Which to me seems like fair play, as they're improving their often commercial products with it, while most vuln apps are open source maintained by volunteers.

@preetkaran20
Copy link

preetkaran20 commented Aug 15, 2020

Yes agreed however I was thinking that say we have just given flags(current approach) and now say ZAP wants to use them then they need to create something similar to the vuln.json with a boolean which states that this flag is useful for them and this flag is not useful for them (say file they named as vuln_zap.json) and they will start using vuln_zap.json file instead, which can reduce the usage and also vuln_zap.json might miss newer updates.

so i was thinking that we can just add a category say Unknown/Not_Categorised and once the users of vulnerableApp use our specification they can tell us the correct category in that case it can help us as well as the scanners and one scanner's work can help others too.

The newer vulnerabilities can have or we can try to have these categories and the older code can be maintained by community.

thanks,
Karan

@bkimminich
Copy link
Member Author

Okay, actually we could throw the flags into different arrays, as already done with type. Then each scanner vendor can still decide which of the lists they want to use. Like:

  • flags
    • source
    • runtime
    • classifiers
    • ...

@preetkaran20
Copy link

preetkaran20 commented Aug 15, 2020

yes yes, i was thinking something similar:

vulnerabilities:
  -
    type:
      - CWE :
        - 79
      - A7:
        - 2017
      - WASC:
         - 8
    flags:
       - source:
            - "main-es[0-9]*.js"
            - "#/search"
       - runtime:
           - "/search"
       - not_categorised:
           - "DomSanitizer"
           - "bypassSecurityTrustHtml"
           - "q="

thanks,
Karan

@bkimminich
Copy link
Member Author

I'm on board with the general idea, except for the unnecessary type split into sub categories. I'll write up a conclusive example in the next days using such a split into different flag types.

@preetkaran20
Copy link

Sure 👍 . However i thought of splitting from the ZAP alerts e.g.
image

Actually i agree with both approaches as both have some pros and cons.
I will rethink on this.

@bkimminich
Copy link
Member Author

bkimminich commented Aug 15, 2020

.vulns.yml (Draft 2)

Here's an updated draft with two XSS examples from OWASP Juice Shop. I've tried to put all know red flags in for both and gave them the best matching specification choosing between code, files and runtime. I'm assuming that all values are going to be interpreted as RegExes, therefore all the escaping. Feedback is welcome as always! 🙏

application:
  name: OWASP Juice Shop
  source: https://github.com/bkimminich/juice-shop
vulnerabilities:
  -
    description: DOM-based XSS in search query
    classifiers:
      - "CWE-79"
      - "CWE-749"
      - "A7:2017"
      - "WASC-8"
    flags:
      files:
        - "main-es[0-9]*\.js"
        - "search-result\.component\.html"
        - "search-result\.component\.ts"
      code:
        - "\[innerhtml\]"
        - "DomSanitizer"
        - "searchValue"
        - "bypassSecurityTrustHtml"
      runtime:
        - "#/search"
        - "q="
  -
    description: Stored XSS in product description
    classifiers:
      - "CWE-79"
      - "CWE-749"
      - "A7:2017"
      - "WASC-8"
    flags:
      files:
        - "main-es[0-9]*\.js"
        - "search-result\.component\.ts"
        - "product-details\.component\.html"
      code:
        - "DomSanitizer"
        - "tableData\[i\]\.description"
        - "bypassSecurityTrustHtml"
        - "trustProductDescription"
        - "\[innerhtml\]"
      runtime:
        - "/api/Products"
        - "/api/Products/[0-9]*"
        - "#/search"

bkimminich added a commit that referenced this issue Aug 15, 2020
(see #1441 for background information and to leave feedback)
@bkimminich
Copy link
Member Author

.vuln.yml (Draft 3)

Here is another option where the flags a grouped by the location they occur in. While this feel to me to be the post accessible way to read this as a human, it also requires the most amount of human work to create... ;-)

application:
  name: OWASP Juice Shop
  source: https://github.com/bkimminich/juice-shop
vulnerabilities:
  -
    description: DOM-based XSS in search query
    classifiers:
      - "CWE-79"
      - "CWE-749"
      - "A7:2017"
      - "WASC-8"
    evidences:
      -
        location: "#/search"
        flags:
          - "q="
      -
        location: "main-es[0-9]*\.js"
        flags:
          - "bypassSecurityTrustHtml"
      -
        location: "search-result\.component\.ts"
        flags:
          - "DomSanitizer"
          - "searchValue"
          - "bypassSecurityTrustHtml"
      -
        location: "search-result\.component\.html"
        flags:
          - "\[innerhtml\]"
          - "searchValue"
  # [...]
  -
    description: SQL Injection into user login
    classifiers:
      - "CWE-74"
      - "CWE-89"
      - "A1:2017"
      - "WASC-19"
    evidences:
      -
        location: "login\.js"
        flags:
          - "sequelize\.query"
          - "req\.body\.email"
          - "req\.body\.password"
      -
        location: "/rest/user/login"
        http_method: POST
        flags:
          - "{email: \".*\", password: \".*\"}"
  # [...]

@preetkaran20
Copy link

Draft 3 looks like has some issues as http_method is not there in first object and as it is a list (thinking from Java prespective) it should be present but with null value. Also how can some scanner know that this location is useful for them. I think it would be great if we can have Code and Runtime added in it.

@bkimminich
Copy link
Member Author

bkimminich commented Aug 19, 2020

I did an attempt with source and url instead of just location, but what is the minified JS file then? It is present in the source folder if the app was built, but it is also delivered by the server. So, it could be seen by DAST and SAST scanners.

And the http_method I only added in one place where it is actually relevant, I meant it to be an optional property. In /#search it wouldn't help much, as that is just a URL fragment (i.e. Angular Route) - anyway, I would just assume that GET (or null?) is the default if nothing is specified.

@preetkaran20
Copy link

preetkaran20 commented Aug 23, 2020

Agreed, js/html/static resources are some kind of exceptions however i think that can reside at both the places with say type.
If we can have type then we can remove GET from SAST/static type and only add to dynamic type.

This will make spec more cleaner as then whatever information for SAST is there in the configuration is relevant for them else they need to remove it or assume it that it is not for them and at runtime it might be tough for them.

I agree this will increase duplicates but i think these cases are only for static resources.

thanks,
Karan

@bkimminich
Copy link
Member Author

bkimminich commented Sep 3, 2020

I've moved the most current drafts and the overall proposal into this GitHub Gist to decouple any discussions from Juice Shop. Please feel free to make suggestions and discuss the drafts over there! Thanks!

@github-actions
Copy link

github-actions bot commented Sep 4, 2021

This thread has been automatically locked because it has not had recent activity after it was closed. 🔒 Please open a new issue for regressions or related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants