Skip to content

Update C test case so it compiles

Craig Smith requested to merge craigmsmith-fix-rules-broken-in-v1.81.0 into main

What does this MR do?

MR Summary

This MR updates:

so that they both compile. This also fixes the semgrep tests that started breaking as of semgrep v1.81.0

Details

As of semgrep v1.81.0 the test for c/crypto/rule-EVP-rc4-40-EVP-rc2-40-cbc.yml and c/crypto/rule-EVP-des-ecb-EVP-des-cbc.c have started failing.

To recreate the failure:

git clone git@gitlab.com:gitlab-org/security-products/sast-rules.git
cd sast-rules

# v1.81.0 will fail
docker run -it --rm -v ${PWD}:/app -w /app returntocorp/semgrep:1.81.0-nonroot semgrep --metrics=off --test --config c/crypto/rule-EVP-rc4-40-EVP-rc2-40-cbc.yml c/crypto/rule-EVP-rc4-40-EVP-rc2-40-cbc.c

# v1.80.0 will pass
docker run -it --rm -v ${PWD}:/app -w /app returntocorp/semgrep:1.80.0-nonroot semgrep --metrics=off --test --config c/crypto/rule-EVP-rc4-40-EVP-rc2-40-cbc.yml c/crypto/rule-EVP-rc4-40-EVP-rc2-40-cbc.c

During the investigation, I noticed that the test file did not compile.

cd sast-rules/c/crypto
docker run -it --rm -v ${PWD}:/usr/src/myapp -w /usr/src/myapp gcc sh
gcc rule-EVP-rc4-40-EVP-rc2-40-cbc.c -o myapp -lssl -lcrypto

This results in the error

rule-EVP-rc4-40-EVP-rc2-40-cbc.c: In function 'main':
rule-EVP-rc4-40-EVP-rc2-40-cbc.c:10:18: error: expected expression before 'void'
   10 |   EVP_rc2_40_cbc(void), EVP_rc2_64_cbc(void), EVP_rc4_40(void);
      |                  ^~~~
rule-EVP-rc4-40-EVP-rc2-40-cbc.c:10:3: error: too many arguments to function 'EVP_rc2_40_cbc'
   10 |   EVP_rc2_40_cbc(void), EVP_rc2_64_cbc(void), EVP_rc4_40(void);
      |   ^~~~~~~~~~~~~~
In file included from rule-EVP-rc4-40-EVP-rc2-40-cbc.c:2:
/usr/include/openssl/evp.h:987:19: note: declared here
  987 | const EVP_CIPHER *EVP_rc2_40_cbc(void);
      |                   ^~~~~~~~~~~~~~
rule-EVP-rc4-40-EVP-rc2-40-cbc.c:10:40: error: expected expression before 'void'
   10 |   EVP_rc2_40_cbc(void), EVP_rc2_64_cbc(void), EVP_rc4_40(void);
      |                                        ^~~~
rule-EVP-rc4-40-EVP-rc2-40-cbc.c:10:25: error: too many arguments to function 'EVP_rc2_64_cbc'
   10 |   EVP_rc2_40_cbc(void), EVP_rc2_64_cbc(void), EVP_rc4_40(void);
      |                         ^~~~~~~~~~~~~~
/usr/include/openssl/evp.h:988:19: note: declared here
  988 | const EVP_CIPHER *EVP_rc2_64_cbc(void);
      |                   ^~~~~~~~~~~~~~
rule-EVP-rc4-40-EVP-rc2-40-cbc.c:10:58: error: expected expression before 'void'
   10 |   EVP_rc2_40_cbc(void), EVP_rc2_64_cbc(void), EVP_rc4_40(void);
      |                                                          ^~~~
rule-EVP-rc4-40-EVP-rc2-40-cbc.c:10:47: error: too many arguments to function 'EVP_rc4_40'
   10 |   EVP_rc2_40_cbc(void), EVP_rc2_64_cbc(void), EVP_rc4_40(void);
      |                                               ^~~~~~~~~~
/usr/include/openssl/evp.h:972:19: note: declared here
  972 | const EVP_CIPHER *EVP_rc4_40(void);

Updating the test file so that it compiles successfully also results in the semgrep tests passing.

What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab/-/issues/476183+s

Does this MR meet the acceptance criteria?

  • The test cases cover both positive and negative cases and have appropriate Semgrep annotations:
    • For positive cases: // ruleid: ...
    • For negative cases: // ok: ....
  • Prefer ($X.servlet.http.HttpServletResponse $RESP).addCookie($C) over $RESPONSE.addCookie($C) to avoid False-Positives.
  • Following metadata fields exist for the rule(s) added/updated in this MR:
    • owasp: with both 2017 and 2021 mappings
    • shortDescription: e.g: "Use of a broken or risky cryptographic algorithm NOT "Use of a Broken or Risky Cryptographic Algorithm"
    • security-severity: one of Info, Low, Medium, High or Critical
    • pattern: use multi-line patterns (with |) only when the actual search patterns spans more than a single line
  • The message contains a secure code example and no insecure ones.
  • The rule is placed in the correct rules/ subfolder based on its license, refering to the internal guidance.
  • Relevant labels including workflow labels are appropriately selected.
  • The MR is freshly rebased with main.
Edited by Craig Smith

Merge request reports