Thread

Re: Enhance security permissions

Bryan Green <dbryan.green@gmail.com> — 2025-11-04T14:35:34Z

On 11/4/2025 7:17 AM, Ranier Vilela wrote:
> Hi.
> 
> Em ter., 4 de nov. de 2025 às 09:44, Bryan Green <dbryan.green@gmail.com
> <mailto:dbryan.green@gmail.com>> escreveu:
> 
>     On 11/4/2025 6:20 AM, Ranier Vilela wrote:
>     > Hi.
>     >
>     > I noticed this while checking the source (src/interfaces/libpq/fe-
>     > connect.c).
>     > It seems that S_IRWXU permission is harmful too.
>     >
>     > In accord with [1] and [2] this should also be checked.
>     > Also, all other places in the source,  S_IRWXU are checked.
>     >
>     > So, I propose adding this check to enhance the security.
>     >
>     > Maybe the error messages, do they need improvement as well?
>     >
>     > patchs attached.
>     >
>     > best regards,
>     > Ranier Vilela
>     >
>     > [1] https://docs.aws.amazon.com/codeguru/detector-library/cpp/
>     loose- <https://docs.aws.amazon.com/codeguru/detector-library/cpp/
>     loose->
>     > file-permissions/ <https://docs.aws.amazon.com/codeguru/detector-
>     <https://docs.aws.amazon.com/codeguru/detector->
>     > library/cpp/loose-file-permissions/>
>     > [2] https://www.exploit-db.com/exploits/33145 <https://
>     www.exploit-db.com/exploits/33145> <https://www.exploit- <https://
>     www.exploit->
>     > db.com/exploits/33145 <http://db.com/exploits/33145>>
>     I just took a glance an you
>     enhance-security-file-permissions-be-secure-common.patch file...
> 
>     I may be misunderstanding either your intent or what this code actually
>     does, but it seems to me that the check rejects files if any of the
>     tested bits are set.
> 
> Correct.
>  
> 
>     Doesn't adding S_IRWXU means rejecting files with
>     any owner permissions, including S_IRUSR (owner read). 
> 
> S_IRWXU on stat is  "Mask for file owner permissions".
>  
> 
>     That would reject
>     mode 0600, which is the documented and required permission for SSL key
>     files.
> 
> I think no.
>

  
(S_IRWXU | S_IRWXG | S_IRWXO) produces a mask of O777, because

S_IRWXU = 0700 (user read, write, execute)
S_IRWXG = 0070 (group read, write, execute)
S_IRWXO = 0007 (other read, write, execute)

mode 0600 & 0777 = 0600 (non-zero)
mode 0400 & 0777 = 0400 (non-zero)
mode 0700 & 0777 = 0700 (non-zero)
mode 0000 & 0777 = 0000 (zero)

The test was originally constructed to reject group and other
permissions being set-- allowing any user(file-owner) permissions to be
accepted.  You are now rejecting everything but 0000.


> 
> 
>     Mode 0000 would be the only thing that passes this check and we can't
>     read that.
> 
>     I believe your [1] reference is about overly permissive roles in
>     creating files.  We are validating existing ones.
> 
> Sorry, I think that [1]  has wrong examples of this.
> 
> [2] has a more correct example.
> 
> We are validating files existing, created by others.
> S_IRWXU file mode indicating readable, writable and executable by owner.
> 
> I think if the file is executable by the owner, He should be rejected,
> shouldn't he?
> 
> best regards,
> Ranier Vilela
> 
> 
> 
> 
> 

If your goal is to reject ONLY executable files, you need to check
S_IXUSR.

However, I question whether even this change is necessary. Private key
files having the execute bit set is not a security vulnerability - the
file cannot actually be executed as a program. The real security concern
is group/world access, which the current code already checks correctly.

BG