[pbs-devel] [PATCH proxmox-backup] proxy: check permissions on proxy.key and proxy.pem files

[pbs-devel] [PATCH proxmox-backup] proxy: check permissions on proxy.key and proxy.pem files

[Gabriel Goller]

Check the owner and permission of the proxy.key and proxy.pem files.
This avoids openssl's unhelpful error message and prints a nicer one.

Motivation: https://forum.proxmox.com/threads/proxmox-backup-tailscale-proxmox-backup-proxy-service-wont-boot.153204

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

Note: not sure about the correct permissions, we currently default to
640, but maybe a minimum of 400 is enough?

 src/bin/proxmox-backup-proxy.rs | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 041f3aff999c..544196b8bc5d 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -367,6 +367,30 @@ async fn run() -> Result<(), Error> {
     Ok(())
 }
 
+/// Check permissions and owner of passed path.
+fn check_permissions<T: AsRef<Path>>(path: T, file_mode: u32) -> Result<(), Error> {
+    match nix::sys::stat::stat(path.as_ref()) {
+        Ok(stat) => {
+            if stat.st_uid != u32::from(pbs_config::backup_user()?.uid)
+                || stat.st_gid != u32::from(pbs_config::backup_group()?.gid)
+                || stat.st_mode & 0o770 < file_mode
+            {
+                bail!(
+                        "file {:?} has wrong permissions - check if it's owned by {}:{} and has {} permissions",
+                        path.as_ref(),
+                        pbs_config::backup_user()?.uid,
+                        pbs_config::backup_group()?.gid,
+                        file_mode
+                    );
+            }
+        }
+        Err(err) => {
+            bail!("unable to open file {:?} - {err}", path.as_ref(),);
+        }
+    }
+    Ok(())
+}
+
 fn make_tls_acceptor() -> Result<SslAcceptor, Error> {
     let key_path = configdir!("/proxy.key");
     let cert_path = configdir!("/proxy.pem");
@@ -375,6 +399,9 @@ fn make_tls_acceptor() -> Result<SslAcceptor, Error> {
     let ciphers_tls_1_3 = config.ciphers_tls_1_3;
     let ciphers_tls_1_2 = config.ciphers_tls_1_2;
 
+    check_permissions(key_path, 0o640)?;
+    check_permissions(cert_path, 0o640)?;
+
     let mut acceptor = proxmox_rest_server::connection::TlsAcceptorBuilder::new()
         .certificate_paths_pem(key_path, cert_path);
 
-- 
2.39.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] proxy: check permissions on proxy.key and proxy.pem files

[Wolfgang Bumiller]

NAK

On Fri, Aug 23, 2024 at 11:12:15AM GMT, Gabriel Goller wrote:
> Check the owner and permission of the proxy.key and proxy.pem files.
> This avoids openssl's unhelpful error message and prints a nicer one.
> 
> Motivation: https://forum.proxmox.com/threads/proxmox-backup-tailscale-proxmox-backup-proxy-service-wont-boot.153204
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> 
> Note: not sure about the correct permissions, we currently default to
> 640, but maybe a minimum of 400 is enough?
> 
>  src/bin/proxmox-backup-proxy.rs | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index 041f3aff999c..544196b8bc5d 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -367,6 +367,30 @@ async fn run() -> Result<(), Error> {
>      Ok(())
>  }
>  
> +/// Check permissions and owner of passed path.
> +fn check_permissions<T: AsRef<Path>>(path: T, file_mode: u32) -> Result<(), Error> {
> +    match nix::sys::stat::stat(path.as_ref()) {
> +        Ok(stat) => {
> +            if stat.st_uid != u32::from(pbs_config::backup_user()?.uid)
> +                || stat.st_gid != u32::from(pbs_config::backup_group()?.gid)
> +                || stat.st_mode & 0o770 < file_mode

If you want to test whether you can open a file, you should either just
`open(2)` it, or, if you really want to avoid it, use `access(2)`.
You do not ever want to attempt to try to perform the kernel's
permission checks yourself. There could be ACLs, AppArmor profiles, ...
and while we can say that, for now, this is not supposed to be the case,
it's bad practice in general.

Also note that this only covers the case at a point in time where the
certificate isn't actually loaded, and won't help with changes to the
permissions while a daemon is already running.

A better approach to handle this specific case would be to adapt
`proxmox-rest-server`'s handling of `Tls::PemFiles` so that instead of
using `openssl`'s ".set_private_key_file()` convenience methods, it
loads the files, and handles `EPERM`/`ENOENT`/... with useful error
messags, and then uses
`acceptor.set_private_key(PKey::private_key_from_pem(data)?)`


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] proxy: check permissions on proxy.key and proxy.pem files

[Gabriel Goller]

On 27.08.2024 11:37, Wolfgang Bumiller wrote:
>NAK
>
>On Fri, Aug 23, 2024 at 11:12:15AM GMT, Gabriel Goller wrote:
>> Check the owner and permission of the proxy.key and proxy.pem files.
>> This avoids openssl's unhelpful error message and prints a nicer one.
>>
>> Motivation: https://forum.proxmox.com/threads/proxmox-backup-tailscale-proxmox-backup-proxy-service-wont-boot.153204
>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>
>> Note: not sure about the correct permissions, we currently default to
>> 640, but maybe a minimum of 400 is enough?
>>
>>  src/bin/proxmox-backup-proxy.rs | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
>> index 041f3aff999c..544196b8bc5d 100644
>> --- a/src/bin/proxmox-backup-proxy.rs
>> +++ b/src/bin/proxmox-backup-proxy.rs
>> @@ -367,6 +367,30 @@ async fn run() -> Result<(), Error> {
>>      Ok(())
>>  }
>>
>> +/// Check permissions and owner of passed path.
>> +fn check_permissions<T: AsRef<Path>>(path: T, file_mode: u32) -> Result<(), Error> {
>> +    match nix::sys::stat::stat(path.as_ref()) {
>> +        Ok(stat) => {
>> +            if stat.st_uid != u32::from(pbs_config::backup_user()?.uid)
>> +                || stat.st_gid != u32::from(pbs_config::backup_group()?.gid)
>> +                || stat.st_mode & 0o770 < file_mode
>
>If you want to test whether you can open a file, you should either just
>`open(2)` it, or, if you really want to avoid it, use `access(2)`.
>You do not ever want to attempt to try to perform the kernel's
>permission checks yourself. There could be ACLs, AppArmor profiles, ...
>and while we can say that, for now, this is not supposed to be the case,
>it's bad practice in general.
>
>Also note that this only covers the case at a point in time where the
>certificate isn't actually loaded, and won't help with changes to the
>permissions while a daemon is already running.
>
>A better approach to handle this specific case would be to adapt
>`proxmox-rest-server`'s handling of `Tls::PemFiles` so that instead of
>using `openssl`'s ".set_private_key_file()` convenience methods, it
>loads the files, and handles `EPERM`/`ENOENT`/... with useful error
>messags, and then uses
>`acceptor.set_private_key(PKey::private_key_from_pem(data)?)`

I agree.
We can use the function you mentioned for the private key (and it
works!), but not for the certificate. AFAICT (and I know nothing about
openssl) there is no use_certificate_chain() function, so the
certificate chain can only be loaded using a path [0]. This seems kinda
weird, as all the other functions have path and file-content flavors,
but this one hasn't.

Nevertheless we could just forget about the certificate or we could do
a open() and close() to check the permissions?

[0]: https://github.com/openssl/openssl/blob/14c45338e986d5827f1e944d0cffe54a7f4697ea/ssl/ssl_rsa.c#L437



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] proxy: check permissions on proxy.key and proxy.pem files

[Wolfgang Bumiller]

On Thu, Aug 29, 2024 at 12:31:22PM GMT, Gabriel Goller wrote:
> On 27.08.2024 11:37, Wolfgang Bumiller wrote:
> > NAK
> > 
> > On Fri, Aug 23, 2024 at 11:12:15AM GMT, Gabriel Goller wrote:
> > > Check the owner and permission of the proxy.key and proxy.pem files.
> > > This avoids openssl's unhelpful error message and prints a nicer one.
> > > 
> > > Motivation: https://forum.proxmox.com/threads/proxmox-backup-tailscale-proxmox-backup-proxy-service-wont-boot.153204
> > > 
> > > Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> > > ---
> > > 
> > > Note: not sure about the correct permissions, we currently default to
> > > 640, but maybe a minimum of 400 is enough?
> > > 
> > >  src/bin/proxmox-backup-proxy.rs | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> > > index 041f3aff999c..544196b8bc5d 100644
> > > --- a/src/bin/proxmox-backup-proxy.rs
> > > +++ b/src/bin/proxmox-backup-proxy.rs
> > > @@ -367,6 +367,30 @@ async fn run() -> Result<(), Error> {
> > >      Ok(())
> > >  }
> > > 
> > > +/// Check permissions and owner of passed path.
> > > +fn check_permissions<T: AsRef<Path>>(path: T, file_mode: u32) -> Result<(), Error> {
> > > +    match nix::sys::stat::stat(path.as_ref()) {
> > > +        Ok(stat) => {
> > > +            if stat.st_uid != u32::from(pbs_config::backup_user()?.uid)
> > > +                || stat.st_gid != u32::from(pbs_config::backup_group()?.gid)
> > > +                || stat.st_mode & 0o770 < file_mode
> > 
> > If you want to test whether you can open a file, you should either just
> > `open(2)` it, or, if you really want to avoid it, use `access(2)`.
> > You do not ever want to attempt to try to perform the kernel's
> > permission checks yourself. There could be ACLs, AppArmor profiles, ...
> > and while we can say that, for now, this is not supposed to be the case,
> > it's bad practice in general.
> > 
> > Also note that this only covers the case at a point in time where the
> > certificate isn't actually loaded, and won't help with changes to the
> > permissions while a daemon is already running.
> > 
> > A better approach to handle this specific case would be to adapt
> > `proxmox-rest-server`'s handling of `Tls::PemFiles` so that instead of
> > using `openssl`'s ".set_private_key_file()` convenience methods, it
> > loads the files, and handles `EPERM`/`ENOENT`/... with useful error
> > messags, and then uses
> > `acceptor.set_private_key(PKey::private_key_from_pem(data)?)`
> 
> I agree.
> We can use the function you mentioned for the private key (and it
> works!), but not for the certificate. AFAICT (and I know nothing about
> openssl) there is no use_certificate_chain() function, so the
> certificate chain can only be loaded using a path [0]. This seems kinda
> weird, as all the other functions have path and file-content flavors,
> but this one hasn't.
>
> Nevertheless we could just forget about the certificate or we could do
> a open() and close() to check the permissions?

That's unfortunate. However, I think it's usually only the key that gets
strict permissions, not the certificate, since it's meant to be public
anyway.

In theory, I think `.set_certificate()` + `.add_extra_chain_cert()`
should work iterating through the certificates from
`X509::stack_from_pem()`, but this would need careful testing to make
sure the full chain is properly visible.

Simply doing an `open()+close()` before calling the
`set_certificate_chain_file()` is probably okay for now.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup] proxy: check permissions on proxy.key and proxy.pem files

[Gabriel Goller]

On 29.08.2024 13:22, Wolfgang Bumiller wrote:
>On Thu, Aug 29, 2024 at 12:31:22PM GMT, Gabriel Goller wrote:
>> On 27.08.2024 11:37, Wolfgang Bumiller wrote:
>> > NAK
>> >
>> > On Fri, Aug 23, 2024 at 11:12:15AM GMT, Gabriel Goller wrote:
>> > > Check the owner and permission of the proxy.key and proxy.pem files.
>> > > This avoids openssl's unhelpful error message and prints a nicer one.
>> > >
>> > > Motivation: https://forum.proxmox.com/threads/proxmox-backup-tailscale-proxmox-backup-proxy-service-wont-boot.153204
>> > >
>> > > Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> > > ---
>> > >
>> > > Note: not sure about the correct permissions, we currently default to
>> > > 640, but maybe a minimum of 400 is enough?
>> > >
>> > >  src/bin/proxmox-backup-proxy.rs | 27 +++++++++++++++++++++++++++
>> > >  1 file changed, 27 insertions(+)
>> > >
>> > > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
>> > > index 041f3aff999c..544196b8bc5d 100644
>> > > --- a/src/bin/proxmox-backup-proxy.rs
>> > > +++ b/src/bin/proxmox-backup-proxy.rs
>> > > @@ -367,6 +367,30 @@ async fn run() -> Result<(), Error> {
>> > >      Ok(())
>> > >  }
>> > >
>> > > +/// Check permissions and owner of passed path.
>> > > +fn check_permissions<T: AsRef<Path>>(path: T, file_mode: u32) -> Result<(), Error> {
>> > > +    match nix::sys::stat::stat(path.as_ref()) {
>> > > +        Ok(stat) => {
>> > > +            if stat.st_uid != u32::from(pbs_config::backup_user()?.uid)
>> > > +                || stat.st_gid != u32::from(pbs_config::backup_group()?.gid)
>> > > +                || stat.st_mode & 0o770 < file_mode
>> >
>> > If you want to test whether you can open a file, you should either just
>> > `open(2)` it, or, if you really want to avoid it, use `access(2)`.
>> > You do not ever want to attempt to try to perform the kernel's
>> > permission checks yourself. There could be ACLs, AppArmor profiles, ...
>> > and while we can say that, for now, this is not supposed to be the case,
>> > it's bad practice in general.
>> >
>> > Also note that this only covers the case at a point in time where the
>> > certificate isn't actually loaded, and won't help with changes to the
>> > permissions while a daemon is already running.
>> >
>> > A better approach to handle this specific case would be to adapt
>> > `proxmox-rest-server`'s handling of `Tls::PemFiles` so that instead of
>> > using `openssl`'s ".set_private_key_file()` convenience methods, it
>> > loads the files, and handles `EPERM`/`ENOENT`/... with useful error
>> > messags, and then uses
>> > `acceptor.set_private_key(PKey::private_key_from_pem(data)?)`
>>
>> I agree.
>> We can use the function you mentioned for the private key (and it
>> works!), but not for the certificate. AFAICT (and I know nothing about
>> openssl) there is no use_certificate_chain() function, so the
>> certificate chain can only be loaded using a path [0]. This seems kinda
>> weird, as all the other functions have path and file-content flavors,
>> but this one hasn't.
>>
>> Nevertheless we could just forget about the certificate or we could do
>> a open() and close() to check the permissions?
>
>That's unfortunate. However, I think it's usually only the key that gets
>strict permissions, not the certificate, since it's meant to be public
>anyway.
>
>In theory, I think `.set_certificate()` + `.add_extra_chain_cert()`
>should work iterating through the certificates from
>`X509::stack_from_pem()`, but this would need careful testing to make
>sure the full chain is properly visible.

Yes, this would be quite tricky to get right.

>Simply doing an `open()+close()` before calling the
>`set_certificate_chain_file()` is probably okay for now.

Will do!
Posted a v2!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel