Support `owasp` mapping key for SecureCodeWarrior to Vulnerability Details and Modal

The SecureCodeWarrior API supports other mapping keys other than CWE, let's ensure we display those corresponding training materials.

The complete list of supported mapping list for SecureCodeWarrior can be called via this API request:

https://integration-api.securecodewarrior.com/docs/#/definitions/MappingListResponse

curl -X GET 'https://integration-api.securecodewarrior.com/api/v1/mapping-lists'

Sample Response

[
  {
    "slug": "cwe",
    "name": "Common Weakness Enumeration (CWE)",
    "_links": {
      "detail": "https://integration-api.securecodewarrior.com/api/v1/mapping-lists/cwe/items"
    }
  },
  {
    "slug": "default",
    "name": "Secure Code Warrior",
    "_links": {
      "detail": "https://integration-api.securecodewarrior.com/api/v1/mapping-lists/default/items"
    }
  },
  {
    "slug": "owasp-api-2019",
    "name": "OWASP API Top 10 2019",
    "_links": {
      "detail": "https://integration-api.securecodewarrior.com/api/v1/mapping-lists/owasp-api-2019/items"
    }
  },
  {
    "slug": "owasp-mobile-2016",
    "name": "OWASP TOP 10 Mobile - 2016",
    "_links": {
      "detail": "https://integration-api.securecodewarrior.com/api/v1/mapping-lists/owasp-mobile-2016/items"
    }
  },
  {
    "slug": "owasp-web-2017",
    "name": "OWASP TOP 10 Web - 2017: The Ten Most Critical Web Application Security Risks",
    "_links": {
      "detail": "https://integration-api.securecodewarrior.com/api/v1/mapping-lists/owasp-web-2017/items"
    }
  },
  {
    "slug": "phrase",
    "name": "Phrase",
    "_links": {
      "detail": "https://integration-api.securecodewarrior.com/api/v1/mapping-lists/phrase/items"
    }
  },
  {
    "slug": "vrt",
    "name": "Vulnerability Rating Taxonomy (VRT) from Bugcrowd",
    "_links": {
      "detail": "https://integration-api.securecodewarrior.com/api/v1/mapping-lists/vrt/items"
    }
  },
  {
    "slug": "vulncat",
    "name": "Fortify vulnerability categories (vulncat) from MicroFocus",
    "_links": {
      "detail": "https://integration-api.securecodewarrior.com/api/v1/mapping-lists/vulncat/items"
    }
  }
]

This support will need to be added to the following pages:

1. Vulnerability Details page
2. Vulnerability Modal in MR widget and pipeline security tab

Order of importance for support

We want to add support for this identifier

1. owasp (We will try to tackle this only for lower case owasp, the upper case one will be tackled in #366556)

Note

As for our other training provider, Kontra, only CWE is supported, so this additional check will not be applicable.

added to epic &6176 (closed)

mentioned in epic &6176 (closed)

added devopssecure frontend groupthreat insights typefeature labels

changed milestone to %14.9

set weight to 2

removed weight

set weight to 3

changed title from Support other mapping key for SecureCodeWarrior to Support other mapping key for SecureCodeWarrior to Vulnerability Details and Modal

changed the description

added sectionsec label

added [deprecated] Accepting merge requests label

made the issue confidential

Making this issue confidential per > https://gitlab.com/gitlab-org/gitlab/-/issues/346896#note_777995376

@matt_wilson apologies, I should have cc'd you on this when this issue was created I don't think this will be a deal-breaker for the first release though -- this only affects one of our provider and I feel we can tackle this iteratively and add the additional vulnerability types support in the following releases

cc @dpisek @andyvolpe

@sming-gitlab Agreed, this is definitely not a deal-breaker. CWE is the most important as it is the most universal (and both launch partners use it). SCW supports a lot more options than I realized. The OWASP mappings are definitely the next most important. Phrase could be useful in the event that we don't have an identifier that matches one of their accepted schemes but seems like it will be harder for us to use as it looks like it expects specific values and isn't a free text/pattern match. As for vrt and vulncat, I'm not sure if we even have analyzers that output those.

@matt_wilson

As for vrt and vulncat, I'm not sure if we even have analyzers that output those.

I had never heard of these before and of course couldn't stop myself from going down the . here is a TL/DR:

VRT (Vulnerability Rating Taxonomy): Bugcrowds proprietary, (albeit adjacent to CWE/OWASP/CVSS) vulnerability taxonomy and scoring system for common weaknesses. Used in their crowdsourced bug bounty program. VRT is open-source and maintained in GitHub.

Vulncat (Fortify {Microfocus} Taxonomy of Software Security Vulnerabilities): Used by Fortify SCA tools and managed services. Another adjacent taxonomy to OWASP/SANS/CWE

@andyvolpe Thanks for digging into this! Sounds like these both are best left unimplemented until such time as there is a clear customer need.

Fantastic! Thank you both for providing the extra context, I'll update the description to account for this information

changed the description

mentioned in issue sming-gitlab/my-gitlab#20 (closed)

@sming-gitlab Looking at this again, I think Phrase should be removed from consideration or at least split into a separate issue. Since you have to supply specific strings and not free text, that starts more tightly coupling us to SCW's API as I doubt the same strings will be used by other providers. I'm updating the Description to take this out for now.

Gotcha, great call I've created a follow up to capture Phrase support > https://gitlab.com/gitlab-org/gitlab/-/issues/353237 Thanks for updating the description

changed the description

mentioned in merge request !81658 (merged)

I'm changing this to a backend since they are taking care of the vendor request

fyi @subashis

Thanks @sming-gitlab, I will have a look into this.

added backend label

changed milestone to %Backlog

Adding label for refinement.

@matt_wilson When are we planning to introduce more mapping keys(external types) for SecureCodeWarrior (Kontra does not have that support right now) ?

/cc @thiagocsf

@subashis This would be a great fast-follow. @thiagocsf if we have capacity for %14.10, let's pull this in. Otherwise, let's look for the next milestone with bandwidth.

@matt_wilson, added it - I'm yet to do planning, so there's still a chance it might get bumped.

@subashis, are there any other related issues to this that need to be scheduled? I don't see any issues blocked by or related to this, but I don't know enough about the implementation to be sure that there's no FE work required.

Thanks @matt_wilson @thiagocsf.

I don't see any issues blocked by or related to this, but I don't know enough about the implementation to be sure that there's no FE work required.

It depends on the implementation plan whether we will need separate FE issue or not. I will look into the refinement and if necessary will create another FE/BE issue.

added workflowrefinement label

changed milestone to %14.10

@minac, could you please refine this issue?

Bot policy.

assigned to @minac

Thanks for working on this @(confidential)! We've removed the Seeking community contributions label to avoid having multiple people working on the same issue.

removed [deprecated] Accepting merge requests label

As I am looking into the implementation plan already, assigning it to myself.

assigned to @subashis

unassigned @minac

Right now, we have in GraphQL something like this where we assumed the mapping key(external type) by default will be cwe:

{
 project(fullPath: "root/security-reports-test") {
  securityTrainingsUrls(identifierExternalIds: ["91", "94"]) {
   name
   url
   status
  }
 }
}

In order to support more mapping keys, I am thinking of adding one more argument of identifierExternalType. The query will look like this:

{
 project(fullPath: "root/security-reports-test") {
  securityTrainingsUrls(identifierExternalType: 'cwe'/another mapping key, identifierExternalIds: ["91", "94"]) {
   name
   url
   status
  }
 }
}

For every external type frontend will send one request with all the identifierExternalIds for that type. I know we are already finding cwe identifiers for current GraphQL API request.

WDYT @sming-gitlab @dpisek?

If we agree I will update the implementation plan and will also create another issue for FE to make the changes related to support other mapping keys.

Thanks @sming-gitlab for creating the frontend issue for this. I am removing the frontend tag from this issue.

Thanks again for the productive braining storming @subashis @dpisek Here's the summary of our talk for us to simmer and investigate the best approach forward

Notes

Say if we have multiple identifiers: CWE-91 and OWASP-91, we want to ensure we have the type associated with the number. So the frontend will join these two fields externalType and externalId and the backend will parse the necessary information and decide whether it's a supported identifier and make the API call. The benefit of having the backend do the processing will be one single source of truth for maintenance and implementation.

{
  project(fullPath: "root/security-reports-test") {
  securityTrainingsUrls(identifiers: ["cwe-91", "owasp-91"]) {
    name
    url
    status
    }
  }
}

This does change our current implementation, so perhaps a more graceful approach is to introduce this argument as a new field

query getSecurityTrainingUrls($projectFullPath: ID!, 
  identifiers: ["CWE-91", "OWASP-91"] // 👈 new field
  $identifierExternalIds: [String!]!) { // 👈 existing field to deprecate
  project(fullPath: $projectFullPath) {
    id
    securityTrainingUrls(identifierExternalIds: $identifierExternalIds) {
      name
      status
      url
    }
  }
}

Note: my code syntax might be a bit off, so please excuse

Thanks @sming-gitlab. I will have a look into prod db to have a better sense about identifiers data. After that I will have another look into the implementation plan from backend side before finishing the refinement.

@subashis thank you! Oh, one thing we might want to take into consideration is this issue of bad data > #357008 (comment 893390136). The reliance of externalId might be a bit risky, perhaps also taking exploring the name field might be a better idea. But I'll let you investigate that further before jumping to conclusion

@nmccorrison fyi: this implementation would also resolve the problem mentioned in this issue > #357008 (comment 893502013). Perhaps we should channel our energy on this issue only, so we can hit two birds with one stone (no bird was hurt in this comment ).

Thanks @sming-gitlab. This is good news, and reinforces moving this to the top of our priority list. Would you agree?

backend will parse the necessary information and decide whether it's a supported identifier

This lines up with my question/proposal in #357008 (comment 893502013)

Thanks @sming-gitlab.

The reliance of externalId might be a bit risky, perhaps also taking exploring the name field might be a better idea.

Before this we need to make sure we can rely on name. If name does not include correct external_id we can not rely on that either. Do you know @sming-gitlab for your investigation whether we can always rely on name?

I agree with @nmccorrison about moving this to the top of our priority list, we can work on this issue in %14.10 instead of Provide vulnerability file language type. WDYT @sming-gitlab?

/cc @thiagocsf

Agree completely, let's do a switch-a-roo, and tackle this first

@subashis It seems to me the bug, like you said, is just bad data; it's definitely a unique situation. It sucks that we have un-reliant data, but it's not something we can control right, I'm guessing we get that information from another API provider

But I do feel like the name would be our best bet, just because it is what is displayed to our users and will align with their expectations. If they see it's CWE, they would expect to see a CWE related training link. Even if the name data is incorrect, say Gosec (but in reality, it's cwe), they would not expect to see a training link -- so the outcome matches their expectation

We could stick with just passing the name as the argument, but I could also pass all 3 arguments with some sort of separator (or maybe just 2 include <name>-<externalType>); if you think helps the backend with a faster parsing

@sming-gitlab @subashis can you provide a few examples of bad externalId data? We can then pass those on to the appropriate analyzer teams to investigate.

Thanks for the tag, @subashis. Please update the milestone as needed.

@nmccorrison currently we have just one flagged issue But great idea about raising that for the analyzer team to investigate this and hopefully fix it. I'm opening up an issue now

#357418 (closed)

We could stick with just passing the name as the argument, but I could also pass all 3 arguments with some sort of separator (or maybe just 2 include <name>-<externalType>); if you think helps the backend with a faster parsing

@sming-gitlab I am still investigating, I am seeing some name for cwe where there is no way to derive the mapping key like: "ASSERT", "Uncaught-exception", "Index-out-of-bounds", "Heap-buffer-overflow\nREAD 1", "Index-out-of-range", "OWASP Dependency-Check", "Floating-point-exception", "Heap-buffer-overflow\nWRITE 1", "Stack-buffer-overflow\nWRITE 4", "Heap-buffer-overflow\nREAD 2". Still need to look into the other external types and mapping key/external id.

I will also have another look into api doc for mapping keys.

Discussion about using name to derive mapping key instead of external_id #357418 (comment 896434681) for cwe.

Other than cwe(which is implemented before, still need more changes), it looks like from the description and discussion, we are going for OWASP as part of this issue.

Mapping keys that are available in Secure Code Warrior right now:

curl -X GET 'https://integration-api.securecodewarrior.com/api/v1/mapping-lists/owasp-web-2017/items'

[{:mappingKey=>"A1", :name=>"A1. Injection"},
 {:mappingKey=>"A10", :name=>"A10. Insufficient Logging And Monitoring"},
 {:mappingKey=>"A2", :name=>"A2. Broken Authentication"},
 {:mappingKey=>"A3", :name=>"A3. Sensitive Data Exposure"},
 {:mappingKey=>"A4", :name=>"A4. XML External Entities (XXE)"},
 {:mappingKey=>"A5", :name=>"A5. Broken Access Control"},
 {:mappingKey=>"A6", :name=>"A6. Security Misconfiguration"},
 {:mappingKey=>"A7", :name=>"A7. Cross-Site Scripting"},
 {:mappingKey=>"A8", :name=>"A8. Insecure Deserialization"},
 {:mappingKey=>"A9", :name=>"A9. Using Components With Known Vulnerabilities"}]

curl -X GET 'https://integration-api.securecodewarrior.com/api/v1/mapping-lists/owasp-mobile-2016/items'

[{:mappingKey=>"M1", :name=>"M1. Improper platform usage"},
 {:mappingKey=>"M10", :name=>"M10. Extraneous functionality"},
 {:mappingKey=>"M2", :name=>"M2. Insecure data storage"},
 {:mappingKey=>"M3", :name=>"M3. Insecure Communication"},
 {:mappingKey=>"M4", :name=>"M4. Insecure Authentication"},
 {:mappingKey=>"M5", :name=>"M5. Insufficient cryptography"},
 {:mappingKey=>"M6", :name=>"M6. Insecure Authorization"},
 {:mappingKey=>"M7", :name=>"M7. Client Code Quality"},
 {:mappingKey=>"M8", :name=>"M8. Code tampering"},
 {:mappingKey=>"M9", :name=>"M9. Reverse engineering"}]

curl -X GET 'https://integration-api.securecodewarrior.com/api/v1/mapping-lists/owasp-api-2019/items'

[{:mappingKey=>"API1", :name=>"API1. Broken Object Level Authorization"},
 {:mappingKey=>"API10", :name=>"API10. Insufficient Logging & Monitoring"},
 {:mappingKey=>"API2", :name=>"API2. Broken Authentication"},
 {:mappingKey=>"API3", :name=>"API3. Excessive Data Exposure"},
 {:mappingKey=>"API4", :name=>"API4. Lack of Resources & Rate Limiting"},
 {:mappingKey=>"API5", :name=>"API5. Broken Function Level Authorization"},
 {:mappingKey=>"API6", :name=>"API6. Mass Assignment"},
 {:mappingKey=>"API7", :name=>"API7. Security Misconfiguration"},
 {:mappingKey=>"API8", :name=>"API8. Injection"},
 {:mappingKey=>"API9", :name=>"API9. Improper Assets Management"}] 

marked this issue as related to #356287 (closed)

mentioned in issue #356287 (closed)

removed frontend label

changed the description

removed weight

changed milestone to %15.0

mentioned in epic &7805 (closed)

changed epic to &7805 (closed)

changed milestone to %14.10

mentioned in issue #346455 (closed)

mentioned in issue #357418 (closed)

mentioned in issue #357569 (closed)

marked this issue as related to #357569 (closed)

mentioned in issue #357008

removed the relation with #357569 (closed)

@sming-gitlab @subashis do we have a couple examples of vulnerabilities that have OWASP identifiers?

@nmccorrison Unfortunately, I haven't spotted any in the wild yet 🦒 Locally, I used this custom script to generate my vulnerabilities > sming-gitlab/my-gitlab#20 (comment 830907756). But probably not what you're hoping for

Maybe I'll wait for Subashis to chime in here; he probably has some super power to scan through our production DB 🦸🤩

Cool, @sming-gitlab. Yes, let's try to find some real vulnerabilities to confirm the pattern.

That script is interesting. Would love to learn more. Is it documented somewhere?

changed title from Support other mapping key for SecureCodeWarrior to Vulnerability Details and Modal to Support {+OWASP+} mapping key for SecureCodeWarrior to Vulnerability Details and Modal

I updated the title to reflect, we are only aiming for OWASP in this issue. We can create separate issue for other mapping keys if we need to add those in future like we added one for phrase.

/cc @sming-gitlab

Agree, good call! Thanks for making the changes

mentioned in issue #360718 (closed)

marked this issue as related to #360718 (closed)

changed milestone to %15.0

added workflowin dev label and removed workflowrefinement label

I have some progress on this but not enough to send an MR. Moving this to %15.1. As per our last chat @sming-gitlab, you are not blocked by this issue. So should be ok to move this to next milestone as I will be away for next two weeks .

/cc @thiagocsf

changed milestone to %15.1

made the issue visible to everyone

changed milestone to %15.2

mentioned in issue #352568 (closed)

I resumed work on this again (finally). When I started looking into it again, I found some challenges on this. The first one is the coordination between frontend and backend. What data should we send in backend and how backend will determine OWASP. @sming-gitlab and I did sync session today to do some brainstorming around this. Thanks @sming-gitlab for these notes sming-gitlab/my-gitlab#30 (comment 1001222917). Posting it here so that we can get this in one place.

I will keep working on finding the mapping between our backend data and these mapping keys.

Another update from a sync session with @sming-gitlab sming-gitlab/my-gitlab#30 (comment 1010331734) about how will approach this.

@sming-gitlab As we created separate backend issue for uppercase OWASP, I will keep this one for lowercase one. I updated the description.

Good call on this, thank you! -- I've also changed the title to reflect this

mentioned in issue #366556

marked this issue as related to #366552 (closed)

mentioned in issue #366552 (closed)

changed the description

changed title from Support {-OWASP-} mapping key for SecureCodeWarrior to Vulnerability Details and Modal to Support {+owasp+} mapping key for SecureCodeWarrior to Vulnerability Details and Modal

@sming-gitlab Does this issue still need to be confidential?

@subashis not at all -- we only made it confidential b/c our vendors weren't announced yet (https://gitlab.com/gitlab-org/gitlab/-/issues/346896#note_777995376). but now that they are let's open this up Thanks for catching this

@sming-gitlab I thought so, was double checking. Thanks for updating the issue.

Removing the blocked by issue as the MR for the issue got merged !92050 (merged).

removed the relation with #366552 (closed)

changed milestone to %15.3

mentioned in merge request !92957 (merged)

mentioned in merge request !93377 (merged)

added workflowin review label and removed workflowin dev label

added workflowverification label and removed workflowin review label

@subashis I was reviewing this while looking at 15.4 planned frontend work. It looks like you may have unintentionally left yourself assigned when moving to verification?

Thanks @nmccorrison. You are right. I will verify it and will close the issue.

@subashis I attempted to verify this quickly yesterday just to help clear down our outstanding verifications but don't seem to be able to pull any security training urls at all strangely.

{
  project(fullPath: "gitlab-org/gitlab") {
  	securityTrainingUrls(identifierExternalIds: ["cwe-91", "OWASP-91"]) {
      name
      url
      status
    }
  }
}

The above query when run in the production graphql explorer doesn't return any results at all. I was not able to identify any instances of the failure in sentry, though, I wouldn't take that as a guarantee that they aren't there.

I was additionally unable to receive security training urls using the same query against my local instance on a project I know successfully queried training urls in the past.

changed milestone to %15.4

Thanks @ghavenga for verifying this.

The format for identifierExternalIds has been changed recently as part of this !92050 (merged). That is why it is not working. You can find the new format in the description section of the MR !92957 (merged).

Example:

https://gitlab.com/gitlab-org/gitlab/uploads/d85135f7d1952d553c264b28ad39d771/2022-08-03_at_9.29_PM.png

Thought this format endpoint is used in ui, but it is public facing, we may need to add that format in the doc https://docs.gitlab.com/ee/api/graphql/reference/#projectsecuritytrainingurls, I will add an issue for that.

I created this issue #372898 to track doc change.

FYI @sming-gitlab

Fantastic. Thank you for the additional information @subashis. I was not aware of the format change. With that in hand I was able to appropriately verify that this is working as intended in production. Closing accordingly.

Thanks @ghavenga again for verifying and closing this issue. Glad that it is working as expected.

mentioned in issue #372898

closed

mentioned in issue gitlab-org/quality/triage-reports#9116 (closed)