1. @ihsinme @geoffw0 Could the message be improved, like in the query above, to point where chmod or fopen is used?
2. In the query above:
   
   Isn't the ql designed to skip the result?

  fc.getTarget().hasGlobalOrStdName("umask") and
  fc.getArgument(0).getValue() = "0" and
  not exists(FunctionCall fctmp |
    fctmp.getTarget().hasGlobalOrStdName("umask") and
    fctmp.getArgument(0).getValue() != "0"
  ) and

Good day @geoffw0
On the issue of including information about fopen or chmod in the message.
for fopen this is not very good, because we are looking for a wide mask, and there can be many places for calling function fopen.
for chmod this is reasonable enough, but it will require the introduction of additional variables. as an option, I propose to consider replacing the detector, in this block from umask to chmod

I propose to consider replacing the detector, in this block from umask to chmod

For the second case in your query I think this might be sensible.

Its possible to report both locations with something along the lines of:

select firstElement, "This relates to $@.", secondElement, secondElement.toString()

This is tricky to do when you sometimes do and sometimes don't have that secondElement (you end up having a secondElement regardless but assigning it to a harmless value when you don't have $@ in the string).

if you agree, then I will change the request like this.

https://lgtm.com/query/9071199583040934385/

https://lgtm.com/query/9071199583040934385/

Looks reasonable to me.

@geoffw0
I made changes.
can you run a check?

I made changes. can you run a check?

Running...

Please don't merge yet, WIP :)

@ihsinme Correct me if I'm wrong, but the vulnerability pattern is valid when functions like fopen create the file with incorrect permissions. This requires "a", "a+" or O_CREAT to be specified. Do I miss any other scenario?
Looking at results for https://github.com/cottsay/openelp for example, shouldn't it exclude the fopen(file, "r") reference as safe?
In some cases all fopen usages might be safe.

@JarLob
in the first block of this query, we find work with a wide mask, without narrowing it after. if there are functions for opening the file.
not important for writing or reading.
In fact, we are looking for files that open with the ability to access them from users in other groups.
this creates the possibility of influencing accessibility (using links and overwriting third-party files on behalf of the program), integrity (the ability to change files) and confidentiality (access to files)

As I understand from umask documentation this is about a process file mode creation default mask. The file creation (with incorrectly wide file permissions) may happen implicitly during such file operations as fopen when file mode is set to "a" that automatically creates the file if it doesn't exist. However in case of the file mode "r" the fopen is documented to fail if the file doesn't exist, so there is no room for implicit file creation with the default permission mask. If fopen operates on an existing file AFAIK it doesn't change it permissions.

Could you please explain me once more why the combination of fopen(..., "r") and umask(0) is dangerous? You may write in your native language to eliminate any language barrier...

As I understand from umask documentation this is about a process file mode creation default mask. The file creation (with incorrectly wide file permissions) may happen implicitly during such file operations as fopen when file mode is set to "a" that automatically creates the file if it doesn't exist. However in case of the file mode "r" the fopen is documented to fail if the file doesn't exist, so there is no room for implicit file creation with the default permission mask. If fopen operates on an existing file AFAIK it doesn't change it permissions.

Could you please explain me once more why the combination of fopen(..., "r") and umask(0) is dangerous? You may write in your native language to eliminate any language barrier...

good afternoon @JarLob
You're right, there are no security concerns with this combination.
Dear @geoffw0 , tell me how to remove the file reading situation from the detection. I do not find standard approaches in codeql.
Thanks.

tell me how to remove the file reading situation from the detection.

We don't currently have a good model for fopen with details such as this, so I think your best bet is to check the value of the second parameter yourself. Something like:

exists(FunctionCall fc |
  fc.getTarget().hasGlobalOrStdName("fopen") and
  not fc.getArgument(1).getValue().matches("r%") // r or r+
)

not fc.getArgument(1).getValue().matches("r%") // r or r+

Thanks for the answer.
if I do so, do you agree?

  exists(FunctionCall fctmp |
    (
      (
        fctmp.getTarget().hasGlobalOrStdName("fopen") or
        fctmp.getTarget().hasGlobalOrStdName("open")
      ) and
      (
        not fctmp.getArgument(1).getValue().matches("r%") and
        not fctmp.getArgument(1).getValue().matches("O_RDONLY")
      )
    ) and

O_RDONLY is probably a macro or enum constant access, .getValue() will return a string representation of its value (like "0" or "1" or something) not of the access. Other than that your code looks fine.

good afternoon @geoffw0 and @JarLob.
please see the changes made.