Re: Enhance security permissions

On 11/4/2025 7:17 AM, Ranier Vilela wrote:
> Hi.
>
> Em ter., 4 de nov. de 2025 às 09:44, Bryan Green <dbryan(dot)green(at)gmail(dot)com
> <mailto:dbryan(dot)green(at)gmail(dot)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