Обсуждение: Enhance security permissions

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-
> file-permissions/ <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>
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. Doesn't adding S_IRWXU means rejecting files with
any owner permissions, including S_IRUSR (owner read). That would reject
mode 0600, which is the documented and required permission for SSL key
files.

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.

Please correct my understanding as needed.

Thanks,
Bryan

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