Fixed the dockerfile.security.missing-user rule #3438
Conversation
|
|
saghaulor
force-pushed
the
fix_dockerfile_missing_user
branch
3 times, most recently
from
965c154
to
0aede95
Compare
- Fixed a bug where the previous version of this rule would have false positives on ``` HEALTHCHECK ... \ CMD ... ENTRYPOINT ... ``` and ``` HEALTHCHECK ... \ CMD ... ENTRYPOINT ... ``` It doesn't really make sense to flag on the CMD sub-directive of the HEALTHCHECK directive since there's very little chance that the application could be compromised via the HEALTHCHECK and then gain root access. This false positive creates a lot of noise and therefore we're addressing it. - There was a separate rule for ENTRYPOINT, which doesn't really make sense, since CMD and ENTRYPOINT can be used in the same Dockerfile, as per https://docs.docker.com/reference/dockerfile/#exec-form-entrypoint-example Therefore, the rule was removed - Fixed the bug that will create two findings for a Dockerfile like this ``` FROM busybox ENTRYPOINT ["some-command"] CMD ["--some-arg"] ``` - The autofix arguments have changed because technically it doesn't matter where in the Dockerfile the USER directive is specified, insofar as the last specified USER is non-root. Previously, the autofix would attempt to add the USER directive above the CMD or ENTRYPOINT directives. However, since either or both of these can appear, we're not going to specify the CMD or ENTRYPOINT directive in the fix. - Cleaned up some of the test files to remove invalid syntax like calling CMD twice - Fixes semgrep#3436
saghaulor
force-pushed
the
fix_dockerfile_missing_user
branch
from
0aede95
to
283b70f
Compare
|
Hello @saghaulor and thanks for your contribution! I would suggest to keep the rules separated but rewrite the one about the CMD to match it only when ENTRYPOINT is not present:
As you can notice, in the |
|
Also, the way Semgrep's testsuite works is that it will match rule file name and test file name. In your PR, |
|
@p4p3r thanks for the context and the engine fix, I definitely wasn't excited about using regexes for the solution. I just tested it out and it's not longer triggering false positives on the Do you agree that these are valid dockerfiles? FROM busybox
COPY thing /usr/bin/thing
ENTRYPOINT ["/usr/bin/thing"]
USER non-rootFROM busybox
COPY thing /usr/bin/thing
CMD ["/usr/bin/thing"]
USER non-rootMy understanding is that the ENTRYPOINT and/or CMD will be executed as user non-root as it is the last declared user, and if so, there's still a bug in the rule. If my understanding is correct, then that means that there will be false positives related to the above examples. Also, have y'all considered restructuring how your test suite applies to rules? It seems that currently it will necessitate creating new rules for edge cases, or creating test files that are not valid to include edge cases so that you don't have to introduce new rules. Overall, I think most of the false positives have been addressed, but the above examples will still mean that at least one false positive exists (assuming that I have understood dockerfiles correctly). |
|
I've opened a new PR that addresses the bug related to a |
and
It doesn't really make sense to flag on the CMD sub-directive of the HEALTHCHECK
directive since there's very little chance that the application could be compromised
via the HEALTHCHECK and then gain root access. This false positive creates a lot of noise
and therefore we're addressing it.
since CMD and ENTRYPOINT can be used in the same Dockerfile,
as per https://docs.docker.com/reference/dockerfile/#exec-form-entrypoint-example
Therefore, the rule was removed
where in the Dockerfile the USER directive is specified, insofar as the
last specified USER is non-root. Previously, the autofix would attempt
to add the USER directive above the CMD or ENTRYPOINT directives.
However, since either or both of these can appear, we're not going to
specify the CMD or ENTRYPOINT directive in the fix.