[PATCH] package_ipk: Clean up Source entry in ipk packages

[PATCH] package_ipk: Clean up Source entry in ipk packages

[Richard Purdie]

There is the potential for sensitive information to leak through the urls
there and removing it brings this into the behavior of the other package
backends since filtering it is likely error prone.

Since ipks don't appear to be generated at all if we don't set this, set
the field to the recipe name used (basename only, no paths). This avoids
information leaking. We may want to drop the field if opkg can allow that
at a future point but the recipe name is a suitable identifier for now.

Reported-by: Andrej Valek <andrej.valek@siemens.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 meta/classes/package_ipk.bbclass | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/meta/classes/package_ipk.bbclass b/meta/classes/package_ipk.bbclass
index d2ce3b3..8e69b5d 100644
--- a/meta/classes/package_ipk.bbclass
+++ b/meta/classes/package_ipk.bbclass
@@ -57,6 +57,7 @@ def ipk_write_pkg(pkg, d):
 
     outdir = d.getVar('PKGWRITEDIRIPK')
     pkgdest = d.getVar('PKGDEST')
+    recipesource = os.path.basename(d.getVar('FILE'))
 
     localdata = bb.data.createCopy(d)
     root = "%s/%s" % (pkgdest, pkg)
@@ -205,10 +206,7 @@ def ipk_write_pkg(pkg, d):
             ctrlfile.write("Replaces: %s\n" % bb.utils.join_deps(rreplaces))
         if rconflicts:
             ctrlfile.write("Conflicts: %s\n" % bb.utils.join_deps(rconflicts))
-        src_uri = localdata.getVar("SRC_URI").strip() or "None"
-        if src_uri:
-            src_uri = re.sub("\s+", " ", src_uri)
-            ctrlfile.write("Source: %s\n" % " ".join(src_uri.split()))
+        ctrlfile.write("Source: %s\n" % recipesource)
         ctrlfile.close()
 
         for script in ["preinst", "postinst", "prerm", "postrm"]:
-- 
2.7.4

✗ patchtest: failure for package_ipk: Clean up Source entry in ipk packages

[Patchwork]

== Series Details ==

Series: package_ipk: Clean up Source entry in ipk packages
Revision: 1
URL   : https://patchwork.openembedded.org/series/7286/
State : failure

== Summary ==


Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:



* Issue             Series does not apply on top of target branch [test_series_merge_on_head] 
  Suggested fix    Rebase your series on top of targeted branch
  Targeted branch  master (currently at ec7feb9d31)



If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).

---
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe

Re: [PATCH] package_ipk: Clean up Source entry in ipk packages

[Richard Purdie]

On Fri, 2017-06-16 at 09:46 +0100, Richard Purdie wrote:
> There is the potential for sensitive information to leak through the
> urls
> there and removing it brings this into the behavior of the other
> package
> backends since filtering it is likely error prone.
> 
> Since ipks don't appear to be generated at all if we don't set this,
> set
> the field to the recipe name used (basename only, no paths). This
> avoids
> information leaking. We may want to drop the field if opkg can allow
> that
> at a future point but the recipe name is a suitable identifier for
> now.
> 
> Reported-by: Andrej Valek <andrej.valek@siemens.com>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  meta/classes/package_ipk.bbclass | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Since this is rather important I have backported this to
pyro/morty/krogoth with the appropriate tweaks.

Cheers,

Richard

Re: [PATCH] package_ipk: Clean up Source entry in ipk packages

[Denys Dmytriyenko]

On Fri, Jun 16, 2017 at 10:22:35AM +0100, Richard Purdie wrote:
> On Fri, 2017-06-16 at 09:46 +0100, Richard Purdie wrote:
> > There is the potential for sensitive information to leak through the
> > urls
> > there and removing it brings this into the behavior of the other
> > package
> > backends since filtering it is likely error prone.
> > 
> > Since ipks don't appear to be generated at all if we don't set this,
> > set
> > the field to the recipe name used (basename only, no paths). This
> > avoids
> > information leaking. We may want to drop the field if opkg can allow
> > that
> > at a future point but the recipe name is a suitable identifier for
> > now.
> > 
> > Reported-by: Andrej Valek <andrej.valek@siemens.com>
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> >  meta/classes/package_ipk.bbclass | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> Since this is rather important I have backported this to
> pyro/morty/krogoth with the appropriate tweaks.

Ouch! We were actually using that field to generate the URL list for the 
Software Manifest out of the package feed...

Was this discussed before? Can this change be made optional?

-- 
Denys

Re: [PATCH] package_ipk: Clean up Source entry in ipk packages

[Alejandro del Castillo]

On 06/16/2017 03:46 AM, Richard Purdie wrote:
> There is the potential for sensitive information to leak through the urls
> there and removing it brings this into the behavior of the other package
> backends since filtering it is likely error prone.
> 
> Since ipks don't appear to be generated at all if we don't set this, set
> the field to the recipe name used (basename only, no paths). This avoids
> information leaking. We may want to drop the field if opkg can allow that
> at a future point but the recipe name is a suitable identifier for now.

Looking at opkg-build, opkg requires:

	Package, Version, Architecture, Maintainer, Section, Priority, Source

while deb requires:

	Package, Version, Maintainer, Description

It does makes sense to require Architecture, but doesn't make sense to
me to make Section, Priority and Source mandatory. Opkg does process
packages that lack those fields.

This should be a trivial change to opkg-build, which I can submit into
opkg-utils. Including that patch in the opkg-utils recipe may simplify
things here.

> Reported-by: Andrej Valek <andrej.valek@siemens.com>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  meta/classes/package_ipk.bbclass | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/meta/classes/package_ipk.bbclass b/meta/classes/package_ipk.bbclass
> index d2ce3b3..8e69b5d 100644
> --- a/meta/classes/package_ipk.bbclass
> +++ b/meta/classes/package_ipk.bbclass
> @@ -57,6 +57,7 @@ def ipk_write_pkg(pkg, d):
>  
>      outdir = d.getVar('PKGWRITEDIRIPK')
>      pkgdest = d.getVar('PKGDEST')
> +    recipesource = os.path.basename(d.getVar('FILE'))
>  
>      localdata = bb.data.createCopy(d)
>      root = "%s/%s" % (pkgdest, pkg)
> @@ -205,10 +206,7 @@ def ipk_write_pkg(pkg, d):
>              ctrlfile.write("Replaces: %s\n" % bb.utils.join_deps(rreplaces))
>          if rconflicts:
>              ctrlfile.write("Conflicts: %s\n" % bb.utils.join_deps(rconflicts))
> -        src_uri = localdata.getVar("SRC_URI").strip() or "None"
> -        if src_uri:
> -            src_uri = re.sub("\s+", " ", src_uri)
> -            ctrlfile.write("Source: %s\n" % " ".join(src_uri.split()))
> +        ctrlfile.write("Source: %s\n" % recipesource)
>          ctrlfile.close()
>  
>          for script in ["preinst", "postinst", "prerm", "postrm"]:
> 

-- 
Cheers,

Alejandro

Re: [PATCH] package_ipk: Clean up Source entry in ipk packages

[Richard Purdie]

On Fri, 2017-06-16 at 13:43 -0500, Alejandro del Castillo wrote:
> 
> On 06/16/2017 03:46 AM, Richard Purdie wrote:
> > 
> > There is the potential for sensitive information to leak through
> > the urls
> > there and removing it brings this into the behavior of the other
> > package
> > backends since filtering it is likely error prone.
> > 
> > Since ipks don't appear to be generated at all if we don't set
> > this, set
> > the field to the recipe name used (basename only, no paths). This
> > avoids
> > information leaking. We may want to drop the field if opkg can
> > allow that
> > at a future point but the recipe name is a suitable identifier for
> > now.
> Looking at opkg-build, opkg requires:
> 
> 	Package, Version, Architecture, Maintainer, Section, Priority,
> Source
> 
> while deb requires:
> 
> 	Package, Version, Maintainer, Description
> 
> It does makes sense to require Architecture, but doesn't make sense
> to me to make Section, Priority and Source mandatory. Opkg does
> process packages that lack those fields.
> 
> This should be a trivial change to opkg-build, which I can submit
> into opkg-utils. Including that patch in the opkg-utils recipe may
> simplify things here.

I agree, I think that may be a worthwhile change. I was a little
surprised it didn't do that already and appears to silently fail if
Source: isn't set (or we fail to check the exit code).

I also have found out some people are using this Source: data to find
recipe information so we likely do need some way to map it back to the
recipe, I'm hoping the filename is good enough for that purpose...

Cheers,

Richard

Re: [PATCH] package_ipk: Clean up Source entry in ipk packages

[Alejandro del Castillo]

On 06/16/2017 05:11 PM, Richard Purdie wrote:
> On Fri, 2017-06-16 at 13:43 -0500, Alejandro del Castillo wrote:
>>
>> On 06/16/2017 03:46 AM, Richard Purdie wrote:
>>>
>>> There is the potential for sensitive information to leak through
>>> the urls
>>> there and removing it brings this into the behavior of the other
>>> package
>>> backends since filtering it is likely error prone.
>>>
>>> Since ipks don't appear to be generated at all if we don't set
>>> this, set
>>> the field to the recipe name used (basename only, no paths). This
>>> avoids
>>> information leaking. We may want to drop the field if opkg can
>>> allow that
>>> at a future point but the recipe name is a suitable identifier for
>>> now.
>> Looking at opkg-build, opkg requires:
>>
>> 	Package, Version, Architecture, Maintainer, Section, Priority,
>> Source
>>
>> while deb requires:
>>
>> 	Package, Version, Maintainer, Description
>>
>> It does makes sense to require Architecture, but doesn't make sense
>> to me to make Section, Priority and Source mandatory. Opkg does
>> process packages that lack those fields.
>>
>> This should be a trivial change to opkg-build, which I can submit
>> into opkg-utils. Including that patch in the opkg-utils recipe may
>> simplify things here.
> 
> I agree, I think that may be a worthwhile change. I was a little
> surprised it didn't do that already and appears to silently fail if
> Source: isn't set (or we fail to check the exit code).

FWIW the opkg-utils patch is already on the opkg mailing list, in case
you think it's worth pulling it nevertheless.

-- 
Cheers,

Alejandro