michelin/ChopChop

Accept lists of URIs

JulienPalard opened this issue · 9 comments

Thanks for opening ChopChop!

Looked at chopchop.yml and though « I'll gladly add some... », but wanted to do it like so:

- uri: ["/db.sql", "/db.sql.gz", "/db.sqlite", "/db.sqlite.gz", "/db.sqlite3", "/db.sqlite3.gz", "/data.sql", "/data.sql.gz", "/users.sql", "/users.sql.gz", "/dump.sql", "/dump.sql.gz", "/mysqldump.sql", "/mysqldump.sql.gz", "/backup.sql", "/backup.sql.gz", "/db.backup", "/db.backup.gz", "/database.sql", "/database.sql.gz", "/db-data.sql", "/db-data.sql.gz", "/db_test.sql", "/db_test.sql.gz", "/db-test.sql", "/db-test.sql.gz"]
  checks:
    - name: Database file
      status_code: 200
      remediation: Delete this file
      description: Verifies a database dump is accessible.
      severity: "High"

(and I bet we could continue for hours adding to this list)

As you imagine, I don't want to copy/paste the name, status_code, remediation, description, and severity 26 times.

Doodling around the idea, it would be great to be able to express those as a « genex », or something similar, something like:

/(db|database|backup|mysqldump|dump|data).(sql|sqlite|sqlite3)(\.gz)?

Is this a good first issue?

Thanks for opening ChopChop!

Looked at chopchop.yml and though « I'll gladly add some... », but wanted to do it like so:

- uri: ["/db.sql", "/db.sql.gz", "/db.sqlite", "/db.sqlite.gz", "/db.sqlite3", "/db.sqlite3.gz", "/data.sql", "/data.sql.gz", "/users.sql", "/users.sql.gz", "/dump.sql", "/dump.sql.gz", "/mysqldump.sql", "/mysqldump.sql.gz", "/backup.sql", "/backup.sql.gz", "/db.backup", "/db.backup.gz", "/database.sql", "/database.sql.gz", "/db-data.sql", "/db-data.sql.gz", "/db_test.sql", "/db_test.sql.gz", "/db-test.sql", "/db-test.sql.gz"]
  checks:
    - name: Database file
      status_code: 200
      remediation: Delete this file
      description: Verifies a database dump is accessible.
      severity: "High"

(and I bet we could continue for hours adding to this list)

As you imagine, I don't want to copy/paste the name, status_code, remediation, description, and severity 26 times.

Doodling around the idea, it would be great to be able to express those as a « genex », or something similar, something like:

/(db|database|backup|mysqldump|dump|data).(sql|sqlite|sqlite3)(\.gz)?

Status Code 200 will give you 90% false positives for database backups.
A valid fileheader would eliminate this problem.
Not tested tho:

  - uri: "/backup.sql"
    checks:
      - name: SQL Backup
        match:
          - '{"status"'
        headers:
          - "Content-Type:application/sql"
        remediation: Delete it.
        description: Backupfile of your database. Sensitive informations like admin login could be readable.
        severity: "High"

Bonjour @JulienPalard, jamais entendu parler de "genex" auparavant 👴, c'est génial !

I agree with Julien, list of URIs is needed in a lot of use cases.
Implementation of "genex" has also been implemented into Golang : https://github.com/alixaxel/genex

@PaulSec could I help to implement this feature inside ChopChop?

Definitely! Go for it and open a pull request. I will review it ASAP when I will be back from holidays!

Same here ! I'll work on multi-threading and refactor of code in the mean time.

The problem is known in the library we are using, see: go-yaml/yaml#100

Would you find acceptable that the uri parameter is treated as an array all the time? Eg.

  - uri: "/phpinfo.php"
    checks:
      - name: PHPInfo
        match:
          - 'phpinfo()'
        remediation: Disable phpinfo() in PHP.ini
        description: Checks that the phpinfo() function is accessible
        severity: "Low"
        tested: true

Would become:

  - uri: ["/phpinfo.php"]
    checks:
      - name: PHPInfo
        match:
          - 'phpinfo()'
        remediation: Disable phpinfo() in PHP.ini
        description: Checks that the phpinfo() function is accessible
        severity: "Low"
        tested: true

In this case, we wouldn't have any issue in order to unmarshal the data and we could allow array of uri.

It would break already written yml, I don't know if there's any though.

An alternative would be to allow for the uri (an optional string) and the uris (an optional list of strings), to keep compatibility and save three chars when there's a single one:

- uri: "/phpinfo.php"

and

- uris: ["/phpinfo.php", "/phpinfo"]

It raises the ambiguity of giving both, ambiguity is bad, but it's probably simple to resolve : hit both uri and uris, and warn.

I bet if genex are implemented they'll have to be in separated field to, to avoid mis-interpreting paths with strange chars as genex.

Definitely, you're right.
Ok, let's go for two specific fields and hit both on each plugin.

I will write a pull-request and let you know when it's ready (I guess this week-end at least)

Feel free to check out the pull request, it should help you out 🚀