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

ZAP Advanced: Add support for additional report types #1320

Merged
merged 4 commits into from
Aug 31, 2022

Conversation

malexmave
Copy link
Member

@malexmave malexmave commented Aug 26, 2022

ZAP Advanced can be parameterized to return different types of reports. This PR adds the "*-plus" formats that have been added to ZAP (including the XML-plus format that currently has an open pull request against the ZAP Extensions repo). The "plus" reports contain all information of the normal report, plus the full request and response headers and payloads.

While the new format has not yet been approved and merged, it can still be used as follows:

  1. Add the following snippet to the values.yaml of the ZAP advanced scanner, under the extraVolumes key:
    - name: zap-report-format
      configMap:
        name: zap-report-format
        optional: true
  1. Add the following snippet under the extraVolumeMounts of the zapContainer section in the same file:
    - name: zap-report-format
      mountPath: /home/zap/.ZAP/reports/traditional-xml-plus/
      readOnly: true
  1. Create a configmap in the scan namespace with the following contents:
apiVersion: v1
kind: ConfigMap
metadata:
  name: zap-report-format
data:
  report.xml: |-
    <?xml version="1.0"?>
    <OWASPZAPReport
      th:attr="version=${zapVersion}, generated=${generatedString}">
      <th:block th:each="site: ${reportData.sites}">
        <site th:name="${site}"
          th:attr="host=${helper.getHostForSite(site)}, port=${helper.getPortForSite(site)}, ssl=${helper.isSslSite(site)}">
          <alerts>
            <th:block
              th:each="alert: ${helper.getAlertsForSite(alertTree, site)}"
              th:with="instances=${helper.getAlertInstancesForSite(alertTree, site, alert.name, alert.risk)}">
              <alertitem>
                <pluginid th:text="${alert.pluginId}"></pluginid>
                <alertRef th:text="${alert.alertRef}"></alertRef>
                <alert th:text="${alert.name}"></alert>
                <name th:text="${alert.name}"></name>
                <riskcode th:text="${alert.risk}"></riskcode>
                <confidence th:text="${alert.confidence}"></confidence>
                <riskdesc
                  th:text="${helper.getRiskString(alert.risk) + ' (' + helper.getConfidenceString(alert.confidence) + ')'}"></riskdesc>
                <confidencedesc
                  th:text="${helper.getConfidenceString(alert.confidence)}"></confidencedesc>
                <desc
                  th:text="${helper.legacyEscapeParagraph(alert.description)}"></desc>
                <instances>
                  <th:block th:each="instance: ${instances}">
                    <instance>
                      <uri th:text="${instance.uri}"></uri>
                      <method th:text="${instance.method}"></method>
                      <param th:text="${instance.param}"></param>
                      <attack th:text="${instance.attack}"></attack>
                      <evidence th:text="${instance.evidence}"></evidence>
                      <requestheader th:text="${helper.legacyEscapeText(instance.message.requestHeader, true)}"></requestheader>
                      <requestbody th:text="${helper.legacyEscapeText(instance.message.requestBody, true)}"></requestbody>
                      <responseheader th:text="${helper.legacyEscapeText(instance.message.responseHeader, true)}"></responseheader>
                      <responsebody th:text="${helper.legacyEscapeText(instance.message.responseBody, true)}"></responsebody>
                    </instance>
                  </th:block>
                </instances>
                <count th:text="${instances.size()}"></count>
                <solution
                  th:text="${helper.legacyEscapeParagraph(alert.solution)}"></solution>
                <otherinfo
                  th:text="${helper.legacyEscapeParagraph(alert.otherinfo)}"></otherinfo>
                <reference
                  th:text="${helper.legacyEscapeParagraph(alert.reference)}"></reference>
                <cweid th:text="${alert.cweid}"></cweid>
                <wascid th:text="${alert.wascid}"></wascid>
                <sourceid th:text="${alert.sourceHistoryId}"></sourceid>
              </alertitem>
            </th:block>
          </alerts>
        </site>
      </th:block>
    </OWASPZAPReport>
  template.yaml: |
    name: Traditional XML Report with requests and responses
    format: XML
    mode: XML
    extension: xml
  1. Add -r XML-plus to the scan parameters of your ZAP-advanced scan

Note that this will break all other report types, as it will lead to Kubernetes creating the folder /home/zap/.ZAP/reports/traditional-xml-plus/, which will prevent ZAP from unpacking its own set of reports. This issue will go away once the XML-plus report type is officially supported by ZAP and this workaround with the ConfigMap is no longer necessary.

The XML-plus report type is compatible with the existing parser. All other report types are (obviously) incompatible, since the parser expects XML.

Checklist

  • Test your changes as thoroughly as possible before you commit them. Preferably, automate your test by unit/integration tests.
  • Make sure that all your commits are signed-off and that you are added to the Contributors file.
  • Make sure that all CI finish successfully.
  • Optional (but appreciated): Make sure that all commits are Verified.

malexmave and others added 3 commits August 26, 2022 14:33
ZAP reports can come in an enhanced "plus" format, which also contains
the full request and response headers and payload.  This commit adds
support for selecting these formats to ZAP Advanced.

To enable them, simply add '-r XML-plus' to the parameters in the Scan
definition.  The XML-plus format is compatible with the regular XML
format used by the parser.

Signed-off-by: Max Maass <max.maass@iteratec.com>
Signed-off-by: Max Maass <max.maass@iteratec.com>
Signed-off-by: GitHub Actions <securecodebox@iteratec.com>
@malexmave malexmave added enhancement New feature or request scanner Implement or update a security scanner labels Aug 26, 2022
@malexmave malexmave requested a review from J12934 August 26, 2022 12:46
@malexmave malexmave self-assigned this Aug 26, 2022
@github-actions
Copy link

github-actions bot commented Aug 26, 2022

MegaLinter status: ⚠️ WARNING

Descriptor Linter Files Fixed Errors Elapsed time
✅ GIT git_diff yes no 0.17s
⚠️ JAVASCRIPT eslint 1 1 0.64s
✅ JSON eslint-plugin-jsonc 2 0 1.12s
✅ JSON jsonlint 2 0 0.56s
⚠️ JSON prettier 2 1 0.69s
✅ JSON v8r 2 0 4.88s
✅ PYTHON bandit 2 0 0.3s
⚠️ PYTHON black 2 1 0.63s
⚠️ PYTHON flake8 2 57 0.41s
⚠️ PYTHON isort 2 1 0.13s
✅ PYTHON pylint 2 0 1.93s
⚠️ SPELL misspell 6 1 0.04s
✅ YAML prettier 1 0 0.43s
✅ YAML v8r 1 0 1.25s
✅ YAML yamllint 1 0 0.11s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

rseedorff
rseedorff previously approved these changes Aug 27, 2022
@malexmave
Copy link
Member Author

I found that when using this patch, the ZAP Advanced parser will sometimes run into OOM exceptions. On the cluster I am using, the default resource limit appears to be 200 MB of memory. It seems like there is no way to modify the requested resources for the parser using the values.yaml or the parse definition - is this correct? What would be the accepted / correct way of increasing these limits? (ping @J12934)

@J12934
Copy link
Member

J12934 commented Aug 30, 2022

@malexmave parser and hook memory and cpu limits are currently fixed and cannot be changed :(
https://github.com/secureCodeBox/secureCodeBox/blob/main/operator/controllers/execution/scans/parse_reconciler.go#L143-L150

Its possible to add that in the future by adding these attributes to the ParseDefinitions and ScanComlettionHook

See also #1305

@malexmave
Copy link
Member Author

Okay, looks like I'll also have to patch the operator for the current proof of concept deployment as a stopgap measure :(. Thanks for the quick reply.

@malexmave
Copy link
Member Author

Okay, so, one problem with the new report format is that is can get quite large. For example, if you are scanning a single-page application, it may pull the entire JavaScript code into every single request/response pair that it complains about, leading to large result files. For some reason, this seems to be fine for the lurker, but when the parser pulls it down, it will successfully process it, but then fall flat when trying to upload the results:

Starting Parser 
Fetching result file 
Fetched result file 
Transformed raw result file into 17 findings 
Adding UUIDs and Dates to the findings 
Validating Findings. Environment variable CRASH_ON_FAILED_VALIDATION is set to false 
The Findings were successfully validated 
Updated status successfully 
Uploading results to the file storage service 
No response received from FileStorage when uploading finding 
Error [ERR_FR_MAX_BODY_LENGTH_EXCEEDED]: Request body larger than maxBodyLength limit 
    at RedirectableRequest.write (/home/app/parser-wrapper/node_modules/follow-redirects/index.js:102:24)
    at RedirectableRequest.end (/home/app/parser-wrapper/node_modules/follow-redirects/index.js:127:10)
    at dispatchHttpRequest (/home/app/parser-wrapper/node_modules/axios/lib/adapters/http.js:401:11)
    at new Promise (<anonymous>) 
    at httpAdapter (/home/app/parser-wrapper/node_modules/axios/lib/adapters/http.js:48:10)
    at dispatchRequest (/home/app/parser-wrapper/node_modules/axios/lib/core/dispatchRequest.js:58:10)
    at Axios.request (/home/app/parser-wrapper/node_modules/axios/lib/core/Axios.js:108:15)
    at Axios.<computed> [as put] (/home/app/parser-wrapper/node_modules/axios/lib/core/Axios.js:140:17)
    at Function.wrap [as put] (/home/app/parser-wrapper/node_modules/axios/lib/helpers/bind.js:9:15)
    at uploadResultToFileStorageService (/home/app/parser-wrapper/parser-wrapper.js:28:6) {

For reference, the XML file from ZAP is roughly 30 MB in size. Sadly, this failure in the parsing step also prevents the hook step from running, meaning that the files are not sent to DefectDojo :(. If anyone has an idea what is going on here, I'd be open for suggestions.

Axios seems to have an issue where we are hitting a maximum upload size,
even though neither we nor the server set it. This commit explicitly
sets the maximum upload size to unlimited, which seems to fix the issue.

Signed-off-by: Max Maass <max.maass@iteratec.com>
@malexmave
Copy link
Member Author

We discussed the upload issue in todays meeting, and found that it was caused by a problem with Axios. I added a commit that explicitly tells the upload functionality in the parser SDK to accept arbitrarily large uploads, which seems to have fixed the issue for now.

@malexmave malexmave added this to the v3.15.0 milestone Aug 31, 2022
@malexmave malexmave merged commit fe16876 into main Aug 31, 2022
@malexmave malexmave deleted the feature/zap-report-enhancements branch August 31, 2022 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request scanner Implement or update a security scanner
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants