False positive "Uncontrolled data used in path expression" in C code

[irfanHaslanded]

In the evaluation of sysrepo/sysrepo#3353, CodeQL seems to think there is uncontrolled data used in path expression, when there is none.

This argument to a file access function is derived from and then passed to op_export(file_path), which calls fopen(__filename).

https://github.com/sysrepo/sysrepo/pull/3353/files

step_create_input_file is the function responsible to create a unique filename, and is untouched in this diff, and it seems to be not exploitable.

https://github.com/sysrepo/sysrepo/pull/3353/checks?check_run_id=27465095770

[aibaars]

You are right that the alert makes little sense on that pull request. What happened here is that the change in the pull request caused the "fingerprint" of an existing alert to be changed. As a result CodeQL considers the original alert to be "fixed" because its fingerprint is no longer found. However, it also detects an alert with a new fingerprint and wrongly blames the pull request for this.

The old and new version of the same alert can be found via:
https://github.com/sysrepo/sysrepo/security/code-scanning?query=Uncontrolled+data+used+in+path+expression+is%3Aclosed+rule%3Acpp%2Fpath-injection+branch%3Adevel

You might want to re-open the dismissed alert in case you think it is important.

[irfanHaslanded]

Thanks for the explanation. Unfortunately, the link you gave doesn't seem to work for me and leads to 404 Page not found.

IMHO, the code both old and new should not raise this alert as the filepath for fopen is not exploitable by user input.

[aibaars]

Thanks for the explanation. Unfortunately, the link you gave doesn't seem to work for me and leads to 404 Page not found.

I think you need write permissions or security read permission for the repository to view that page. The URL is for the alerts list page and encodes the search terms (CodeQL query and branch), but it is only visible for users with the right permissions. It shows the same alert twice on slightly different lines due to the code change, one marked as fixed yesterday and the other marked as dismissed yesterday .

IMHO, the code both old and new should not raise this alert as the filepath for fopen is not exploitable by user input.

I agree. Technically, the file path comes directly from argv, so it is user-provided. However, you probably don't care, because this is quite common behaviour for CLI tools.

[irfanHaslanded]

Thanks @aibaars . I understand now. CodeQL alerts any filepath change based on argv as user-provided, even if argv is only used to switch filepath between well-defined options.