archive handler: fix suid/sgid flags management

Message ID 2480362c-150e-4ec4-a32e-9bbe47619112@googlegroups.com
State Changes Requested
Headers show
Series
  • archive handler: fix suid/sgid flags management
Related show

Commit Message

Pierluigi Passaro Dec. 3, 2017, 9:03 p.m.
From b9085488b96e3a1984b6cea0a8fbbabe1dd446ce Mon Sep 17 00:00:00 2001
From: Pierluigi Passaro <pierluigi.passaro@gmail.com>
Date: Sun, 3 Dec 2017 21:40:26 +0100
Subject: [PATCH 1/1] archive handler: fix suid/sgid flags management

archive handler calls libarchive with flags set to zero, missing the
correct management of suid/sgid flags.
Set flags to ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER to fix it.

Signed-off-by: Pierluigi Passaro <pierluigi.passaro@gmail.com>
---
 handlers/archive_handler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stefano Babic Dec. 4, 2017, 10:40 a.m. | #1
Hi Pierluigi,

On 03/12/2017 22:03, pierluigi.passaro@gmail.com wrote:
> From b9085488b96e3a1984b6cea0a8fbbabe1dd446ce Mon Sep 17 00:00:00 2001
> From: Pierluigi Passaro <pierluigi.passaro@gmail.com>
> Date: Sun, 3 Dec 2017 21:40:26 +0100
> Subject: [PATCH 1/1] archive handler: fix suid/sgid flags management
> 
> archive handler calls libarchive with flags set to zero, missing the
> correct management of suid/sgid flags.
> Set flags to ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER to fix it.
> 
> Signed-off-by: Pierluigi Passaro <pierluigi.passaro@gmail.com>
> ---
>  handlers/archive_handler.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c
> index ceb60a3..f4821b7 100644
> --- a/handlers/archive_handler.c
> +++ b/handlers/archive_handler.c
> @@ -210,7 +210,7 @@ static int install_archive_image(struct img_type *img,
>  	TRACE("Installing file %s on %s\n",
>  		img->fname, path);
>  
> -	tf.flags = 0;
> +	tf.flags = ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER;
>  
>  	ret = pthread_create(&extract_thread, &attr, extract, &tf);
>  	if (ret) {
> 

NAK. This does not always work and it is the reason why flags are
currently set to 0 (=unsupported).

In fact, restoring permissions and and owner means that the files are
restored with the UID of who generates the archive. But this happens in
the build process (Yocto or Buildroot) and UID is generally different as
the UID of the target.

Flags should not be set in  any circustances: there are projects where
it is ok to not restore permissions, and other ones where it is
required. It must be configurable, that is we need a setup at parser
level (like "restore-perm = true, or whatever) to inform the handler.

Best regards,
Stefano Babic
Pierluigi Passaro Dec. 4, 2017, 12:42 p.m. | #2
Hi Stefano,

On Mon, Dec 4, 2017 at 11:40 AM, Stefano Babic <sbabic@denx.de> wrote:

> Hi Pierluigi,
>
> On 03/12/2017 22:03, pierluigi.passaro@gmail.com wrote:
> > From b9085488b96e3a1984b6cea0a8fbbabe1dd446ce Mon Sep 17 00:00:00 2001
> > From: Pierluigi Passaro <pierluigi.passaro@gmail.com>
> > Date: Sun, 3 Dec 2017 21:40:26 +0100
> > Subject: [PATCH 1/1] archive handler: fix suid/sgid flags management
> >
> > archive handler calls libarchive with flags set to zero, missing the
> > correct management of suid/sgid flags.
> > Set flags to ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER to fix it.
> >
> > Signed-off-by: Pierluigi Passaro <pierluigi.passaro@gmail.com>
> > ---
> >  handlers/archive_handler.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c
> > index ceb60a3..f4821b7 100644
> > --- a/handlers/archive_handler.c
> > +++ b/handlers/archive_handler.c
> > @@ -210,7 +210,7 @@ static int install_archive_image(struct img_type
> *img,
> >       TRACE("Installing file %s on %s\n",
> >               img->fname, path);
> >
> > -     tf.flags = 0;
> > +     tf.flags = ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER;
> >
> >       ret = pthread_create(&extract_thread, &attr, extract, &tf);
> >       if (ret) {
> >
>
> NAK. This does not always work and it is the reason why flags are
> currently set to 0 (=unsupported).
>
> In fact, restoring permissions and and owner means that the files are
> restored with the UID of who generates the archive. But this happens in
> the build process (Yocto or Buildroot) and UID is generally different as
> the UID of the target.
>
> Flags should not be set in  any circustances: there are projects where
> it is ok to not restore permissions, and other ones where it is
> required. It must be configurable, that is we need a setup at parser
> level (like "restore-perm = true, or whatever) to inform the handler.
>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================
>

thanks for your feedback.
Does this mean that swupdate framework already implements a mechanism to
provide parameters to the parser?
If yes, can you point me to any related document/example ?

Thanks
BR
Pier
Stefano Babic Dec. 4, 2017, 2:59 p.m. | #3
Hi Pierluigi,

On 04/12/2017 13:42, Pierluigi Passaro wrote:
> Hi Stefano,
> 
> On Mon, Dec 4, 2017 at 11:40 AM, Stefano Babic <sbabic@denx.de
> <mailto:sbabic@denx.de>> wrote:
> 
>     Hi Pierluigi,
> 
>     On 03/12/2017 22:03, pierluigi.passaro@gmail.com
>     <mailto:pierluigi.passaro@gmail.com> wrote:
>     > From b9085488b96e3a1984b6cea0a8fbbabe1dd446ce Mon Sep 17 00:00:00 2001
>     > From: Pierluigi Passaro <pierluigi.passaro@gmail.com
>     <mailto:pierluigi.passaro@gmail.com>>
>     > Date: Sun, 3 Dec 2017 21:40:26 +0100
>     > Subject: [PATCH 1/1] archive handler: fix suid/sgid flags management
>     >
>     > archive handler calls libarchive with flags set to zero, missing the
>     > correct management of suid/sgid flags.
>     > Set flags to ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER to fix it.
>     >
>     > Signed-off-by: Pierluigi Passaro <pierluigi.passaro@gmail.com
>     <mailto:pierluigi.passaro@gmail.com>>
>     > ---
>     >  handlers/archive_handler.c | 2 +-
>     >  1 file changed, 1 insertion(+), 1 deletion(-)
>     >
>     > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c
>     > index ceb60a3..f4821b7 100644
>     > --- a/handlers/archive_handler.c
>     > +++ b/handlers/archive_handler.c
>     > @@ -210,7 +210,7 @@ static int install_archive_image(struct
>     img_type *img,
>     >       TRACE("Installing file %s on %s\n",
>     >               img->fname, path);
>     >
>     > -     tf.flags = 0;
>     > +     tf.flags = ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER;
>     >
>     >       ret = pthread_create(&extract_thread, &attr, extract, &tf);
>     >       if (ret) {
>     >
> 
>     NAK. This does not always work and it is the reason why flags are
>     currently set to 0 (=unsupported).
> 
>     In fact, restoring permissions and and owner means that the files are
>     restored with the UID of who generates the archive. But this happens in
>     the build process (Yocto or Buildroot) and UID is generally different as
>     the UID of the target.
> 
>     Flags should not be set in  any circustances: there are projects where
>     it is ok to not restore permissions, and other ones where it is
>     required. It must be configurable, that is we need a setup at parser
>     level (like "restore-perm = true, or whatever) to inform the handler.
> 
>     Best regards,
>     Stefano Babic
> 
>     --
>     =====================================================================
>     DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>     HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>     Phone: +49-8142-66989-53 <tel:%2B49-8142-66989-53> Fax:
>     +49-8142-66989-80 <tel:%2B49-8142-66989-80> Email: sbabic@denx.de
>     <mailto:sbabic@denx.de>
>     =====================================================================
> 
> 
> thanks for your feedback.
> Does this mean that swupdate framework already implements a mechanism to
> provide parameters to the parser?

The parser is able to load attributes from the sw-description file. I
have recently added a per entry extension to set specific attributes for
each entry - see "properties". Even if there is thread about the name,
that could change in future.

> If yes, can you point me to any related document/example ?

See in parser.c and swuforwarder.c

Best regards,
Stefano Babic
Pierluigi Passaro Dec. 5, 2017, 9:40 a.m. | #4
Hi Stefano,

On Mon, Dec 4, 2017 at 3:59 PM, Stefano Babic <sbabic@denx.de> wrote:

> Hi Pierluigi,
>
> On 04/12/2017 13:42, Pierluigi Passaro wrote:
> > Hi Stefano,
> >
> > On Mon, Dec 4, 2017 at 11:40 AM, Stefano Babic <sbabic@denx.de
> > <mailto:sbabic@denx.de>> wrote:
> >
> >     Hi Pierluigi,
> >
> >     On 03/12/2017 22:03, pierluigi.passaro@gmail.com
> >     <mailto:pierluigi.passaro@gmail.com> wrote:
> >     > From b9085488b96e3a1984b6cea0a8fbbabe1dd446ce Mon Sep 17 00:00:00
> 2001
> >     > From: Pierluigi Passaro <pierluigi.passaro@gmail.com
> >     <mailto:pierluigi.passaro@gmail.com>>
> >     > Date: Sun, 3 Dec 2017 21:40:26 +0100
> >     > Subject: [PATCH 1/1] archive handler: fix suid/sgid flags
> management
> >     >
> >     > archive handler calls libarchive with flags set to zero, missing
> the
> >     > correct management of suid/sgid flags.
> >     > Set flags to ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER to fix
> it.
> >     >
> >     > Signed-off-by: Pierluigi Passaro <pierluigi.passaro@gmail.com
> >     <mailto:pierluigi.passaro@gmail.com>>
> >     > ---
> >     >  handlers/archive_handler.c | 2 +-
> >     >  1 file changed, 1 insertion(+), 1 deletion(-)
> >     >
> >     > diff --git a/handlers/archive_handler.c
> b/handlers/archive_handler.c
> >     > index ceb60a3..f4821b7 100644
> >     > --- a/handlers/archive_handler.c
> >     > +++ b/handlers/archive_handler.c
> >     > @@ -210,7 +210,7 @@ static int install_archive_image(struct
> >     img_type *img,
> >     >       TRACE("Installing file %s on %s\n",
> >     >               img->fname, path);
> >     >
> >     > -     tf.flags = 0;
> >     > +     tf.flags = ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER;
> >     >
> >     >       ret = pthread_create(&extract_thread, &attr, extract, &tf);
> >     >       if (ret) {
> >     >
> >
> >     NAK. This does not always work and it is the reason why flags are
> >     currently set to 0 (=unsupported).
> >
> >     In fact, restoring permissions and and owner means that the files are
> >     restored with the UID of who generates the archive. But this happens
> in
> >     the build process (Yocto or Buildroot) and UID is generally
> different as
> >     the UID of the target.
> >
> >     Flags should not be set in  any circustances: there are projects
> where
> >     it is ok to not restore permissions, and other ones where it is
> >     required. It must be configurable, that is we need a setup at parser
> >     level (like "restore-perm = true, or whatever) to inform the handler.
> >
> >     Best regards,
> >     Stefano Babic
> >
> >     --
> >     ============================================================
> =========
> >     DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> >     HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> >     Phone: +49-8142-66989-53 <tel:%2B49-8142-66989-53> Fax:
> >     +49-8142-66989-80 <tel:%2B49-8142-66989-80> Email: sbabic@denx.de
> >     <mailto:sbabic@denx.de>
> >     ============================================================
> =========
> >
> >
> > thanks for your feedback.
> > Does this mean that swupdate framework already implements a mechanism to
> > provide parameters to the parser?
>
> The parser is able to load attributes from the sw-description file. I
> have recently added a per entry extension to set specific attributes for
> each entry - see "properties". Even if there is thread about the name,
> that could change in future.
>
> > If yes, can you point me to any related document/example ?
>
> See in parser.c and swuforwarder.c
>
>
I reviewed the following files
- parser/parser.c
- handlers/swuforward_handler.c
As far as I can see, the framework provides the APIs for do it, but
currently does not provide any specific property to manage libarchive flags.
Is this correct?

Best regards,
> Stefano Babic
>
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================
>

Thanks
BR
Pier
Stefano Babic Dec. 5, 2017, 6:57 p.m. | #5
Hi Pierluigi,

On 05/12/2017 10:40, Pierluigi Passaro wrote:
> Hi Stefano,
> 
> On Mon, Dec 4, 2017 at 3:59 PM, Stefano Babic <sbabic@denx.de
> <mailto:sbabic@denx.de>> wrote:
> 
>     Hi Pierluigi,
> 
>     On 04/12/2017 13:42, Pierluigi Passaro wrote:
>     > Hi Stefano,
>     >
>     > On Mon, Dec 4, 2017 at 11:40 AM, Stefano Babic <sbabic@denx.de
>     <mailto:sbabic@denx.de>
>     > <mailto:sbabic@denx.de <mailto:sbabic@denx.de>>> wrote:
>     >
>     >     Hi Pierluigi,
>     >
>     >     On 03/12/2017 22:03, pierluigi.passaro@gmail.com
>     <mailto:pierluigi.passaro@gmail.com>
>     >     <mailto:pierluigi.passaro@gmail.com
>     <mailto:pierluigi.passaro@gmail.com>> wrote:
>     >     > From b9085488b96e3a1984b6cea0a8fbbabe1dd446ce Mon Sep 17
>     00:00:00 2001
>     >     > From: Pierluigi Passaro <pierluigi.passaro@gmail.com
>     <mailto:pierluigi.passaro@gmail.com>
>     >     <mailto:pierluigi.passaro@gmail.com
>     <mailto:pierluigi.passaro@gmail.com>>>
>     >     > Date: Sun, 3 Dec 2017 21:40:26 +0100
>     >     > Subject: [PATCH 1/1] archive handler: fix suid/sgid flags
>     management
>     >     >
>     >     > archive handler calls libarchive with flags set to zero,
>     missing the
>     >     > correct management of suid/sgid flags.
>     >     > Set flags to ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER to
>     fix it.
>     >     >
>     >     > Signed-off-by: Pierluigi Passaro
>     <pierluigi.passaro@gmail.com <mailto:pierluigi.passaro@gmail.com>
>     >     <mailto:pierluigi.passaro@gmail.com
>     <mailto:pierluigi.passaro@gmail.com>>>
>     >     > ---
>     >     >  handlers/archive_handler.c | 2 +-
>     >     >  1 file changed, 1 insertion(+), 1 deletion(-)
>     >     >
>     >     > diff --git a/handlers/archive_handler.c
>     b/handlers/archive_handler.c
>     >     > index ceb60a3..f4821b7 100644
>     >     > --- a/handlers/archive_handler.c
>     >     > +++ b/handlers/archive_handler.c
>     >     > @@ -210,7 +210,7 @@ static int install_archive_image(struct
>     >     img_type *img,
>     >     >       TRACE("Installing file %s on %s\n",
>     >     >               img->fname, path);
>     >     >
>     >     > -     tf.flags = 0;
>     >     > +     tf.flags = ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER;
>     >     >
>     >     >       ret = pthread_create(&extract_thread, &attr, extract,
>     &tf);
>     >     >       if (ret) {
>     >     >
>     >
>     >     NAK. This does not always work and it is the reason why flags are
>     >     currently set to 0 (=unsupported).
>     >
>     >     In fact, restoring permissions and and owner means that the
>     files are
>     >     restored with the UID of who generates the archive. But this
>     happens in
>     >     the build process (Yocto or Buildroot) and UID is generally
>     different as
>     >     the UID of the target.
>     >
>     >     Flags should not be set in  any circustances: there are
>     projects where
>     >     it is ok to not restore permissions, and other ones where it is
>     >     required. It must be configurable, that is we need a setup at
>     parser
>     >     level (like "restore-perm = true, or whatever) to inform the
>     handler.
>     >
>     >     Best regards,
>     >     Stefano Babic
>     >
>     >     --
>     >   
>      =====================================================================
>     >     DENX Software Engineering GmbH,      Managing Director:
>     Wolfgang Denk
>     >     HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
>     Germany
>     >     Phone: +49-8142-66989-53 <tel:%2B49-8142-66989-53> Fax:
>     >     +49-8142-66989-80 <tel:%2B49-8142-66989-80> Email:
>     sbabic@denx.de <mailto:sbabic@denx.de>
>     >     <mailto:sbabic@denx.de <mailto:sbabic@denx.de>>
>     >   
>      =====================================================================
>     >
>     >
>     > thanks for your feedback.
>     > Does this mean that swupdate framework already implements a
>     mechanism to
>     > provide parameters to the parser?
> 
>     The parser is able to load attributes from the sw-description file. I
>     have recently added a per entry extension to set specific attributes for
>     each entry - see "properties". Even if there is thread about the name,
>     that could change in future.
> 
>     > If yes, can you point me to any related document/example ?
> 
>     See in parser.c and swuforwarder.c
> 
> 
> I reviewed the following files
> - parser/parser.c
> - handlers/swuforward_handler.c
> As far as I can see, the framework provides the APIs for do it,

Right.

> but
> currently does not provide any specific property to manage libarchive flags.
> Is this correct?

Yes, as I said, flags are currently unsupported.


Best regards,
Stefano Babic

Patch

diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c
index ceb60a3..f4821b7 100644
--- a/handlers/archive_handler.c
+++ b/handlers/archive_handler.c
@@ -210,7 +210,7 @@  static int install_archive_image(struct img_type *img,
 	TRACE("Installing file %s on %s\n",
 		img->fname, path);
 
-	tf.flags = 0;
+	tf.flags = ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_OWNER;
 
 	ret = pthread_create(&extract_thread, &attr, extract, &tf);
 	if (ret) {