| Message ID | 2480362c-150e-4ec4-a32e-9bbe47619112@googlegroups.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | archive handler: fix suid/sgid flags management | expand |
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
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
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
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
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
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) {