| Message ID | 20200826005527.2696524-1-mail@aparcar.org |
|---|---|
| State | Accepted |
| Delegated to: | Daniel Golle |
| Headers | show |
| Series | build: opkg-key variable key folder | expand |
On 25-08-20, Paul Spooren wrote: > The key folder is used by `opkg` and `usign` to store and retrieve > trusted public keys. Using `opkg-key` outside a running device is > unfeasible as the key folder is hard coded to `/etc/opkg/keys`. > > This commit adds a variable OPKG_KEYS which defaults to `/etc/opkg/keys` > if unset, however allows set arbitrary key folder locations. > > Arbitrary key folder locations are useful to add signature verification > to the ImageBuilders. It should be noted that this increases the attack surface. Especially env variables are easy to set and hard to notice. This could allow an attacker to provide a rogue keyring to opkg by just setting an environment variable, and this would be quite hard to trace. I would prefer one of two alternatives: 1) take a cmdline argument 2) provide different opkg-key scripts for device and host usage. Each script can use a ~hard-coded path to the expected key location (possibly using $TOPDIR etc). While solution 2) would still use env variables for host usage, it would only apply to this host usage of opkg: we do not compromise the security of device-based opkg. Baptiste > Signed-off-by: Paul Spooren <mail@aparcar.org> > --- > package/system/opkg/files/opkg-key | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/package/system/opkg/files/opkg-key b/package/system/opkg/files/opkg-key > index ae5e8a4591..51d1857ad5 100755 > --- a/package/system/opkg/files/opkg-key > +++ b/package/system/opkg/files/opkg-key > @@ -1,5 +1,7 @@ > #!/bin/sh > > +OPKG_KEYS="${OPKG_KEYS:-/etc/opkg/keys}" > + > usage() { > cat <<EOF > Usage: $0 <command> <arguments...> > @@ -19,7 +21,7 @@ opkg_key_verify() { > ( > zcat "$msgfile" 2>/dev/null || > cat "$msgfile" 2>/dev/null > - ) | usign -V -P /etc/opkg/keys -q -x "$sigfile" -m - > + ) | usign -V -P "$OPKG_KEYS" -q -x "$sigfile" -m - > } > > opkg_key_add() { > @@ -27,8 +29,8 @@ opkg_key_add() { > [ -n "$key" ] || usage > [ -f "$key" ] || echo "Cannot open file $1" > local fingerprint="$(usign -F -p "$key")" > - mkdir -p "/etc/opkg/keys" > - cp "$key" "/etc/opkg/keys/$fingerprint" > + mkdir -p "$OPKG_KEYS" > + cp "$key" "$OPKG_KEYS/$fingerprint" > } > > opkg_key_remove() { > @@ -36,7 +38,7 @@ opkg_key_remove() { > [ -n "$key" ] || usage > [ -f "$key" ] || echo "Cannot open file $1" > local fingerprint="$(usign -F -p "$key")" > - rm -f "/etc/opkg/keys/$fingerprint" > + rm -f "$OPKG_KEYS/$fingerprint" > } > > case "$1" in
On 26.08.20 09:17, Baptiste Jonglez wrote: > On 25-08-20, Paul Spooren wrote: >> The key folder is used by `opkg` and `usign` to store and retrieve >> trusted public keys. Using `opkg-key` outside a running device is >> unfeasible as the key folder is hard coded to `/etc/opkg/keys`. >> >> This commit adds a variable OPKG_KEYS which defaults to `/etc/opkg/keys` >> if unset, however allows set arbitrary key folder locations. >> >> Arbitrary key folder locations are useful to add signature verification >> to the ImageBuilders. > It should be noted that this increases the attack surface. > > Especially env variables are easy to set and hard to notice. This could > allow an attacker to provide a rogue keyring to opkg by just setting an > environment variable, and this would be quite hard to trace. The `opkg` process likely runs as root user, so you have so sneak in env variables to a root process, which from my understanding root privileges. Also the current script uses a relative path to `usign`, which means instead of providing your own fake keys via OPKG_KEYS, you could just modify the PATH variable to have a fake version of `usign` running, returning a successful exit code to anything. > I would prefer one of two alternatives: > > 1) take a cmdline argument Where would that cmdline be added? From the ImageBuilder Makefile? In that case it'd go through `opkg` to then be passed to `opkg-key`. This would somewhat force you to use that specific script, while there exists also a `gnupg` version[1]. [1]: https://git.openwrt.org/?p=project/opkg-lede.git;a=blob;f=utils/opkg-key;h=266bb6628a9cae8096f482c4b1d78fe3c3bcb2ca;hb=HEAD > 2) provide different opkg-key scripts for device and host usage. Each > script can use a ~hard-coded path to the expected key location (possibly > using $TOPDIR etc). > > While solution 2) would still use env variables for host usage, it would > only apply to this host usage of opkg: we do not compromise the security > of device-based opkg. I'm somewhat against maintaining two scripts if not really necessary. If we go that road we need absolute path for usign, cat, zcat etc. Paul >> Signed-off-by: Paul Spooren <mail@aparcar.org> >> --- >> package/system/opkg/files/opkg-key | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/package/system/opkg/files/opkg-key b/package/system/opkg/files/opkg-key >> index ae5e8a4591..51d1857ad5 100755 >> --- a/package/system/opkg/files/opkg-key >> +++ b/package/system/opkg/files/opkg-key >> @@ -1,5 +1,7 @@ >> #!/bin/sh >> >> +OPKG_KEYS="${OPKG_KEYS:-/etc/opkg/keys}" >> + >> usage() { >> cat <<EOF >> Usage: $0 <command> <arguments...> >> @@ -19,7 +21,7 @@ opkg_key_verify() { >> ( >> zcat "$msgfile" 2>/dev/null || >> cat "$msgfile" 2>/dev/null >> - ) | usign -V -P /etc/opkg/keys -q -x "$sigfile" -m - >> + ) | usign -V -P "$OPKG_KEYS" -q -x "$sigfile" -m - >> } >> >> opkg_key_add() { >> @@ -27,8 +29,8 @@ opkg_key_add() { >> [ -n "$key" ] || usage >> [ -f "$key" ] || echo "Cannot open file $1" >> local fingerprint="$(usign -F -p "$key")" >> - mkdir -p "/etc/opkg/keys" >> - cp "$key" "/etc/opkg/keys/$fingerprint" >> + mkdir -p "$OPKG_KEYS" >> + cp "$key" "$OPKG_KEYS/$fingerprint" >> } >> >> opkg_key_remove() { >> @@ -36,7 +38,7 @@ opkg_key_remove() { >> [ -n "$key" ] || usage >> [ -f "$key" ] || echo "Cannot open file $1" >> local fingerprint="$(usign -F -p "$key")" >> - rm -f "/etc/opkg/keys/$fingerprint" >> + rm -f "$OPKG_KEYS/$fingerprint" >> } >> >> case "$1" in
On Wed, Aug 26, 2020 at 11:57:55AM -1000, Paul Spooren wrote: > > On 26.08.20 09:17, Baptiste Jonglez wrote: > > On 25-08-20, Paul Spooren wrote: > > > The key folder is used by `opkg` and `usign` to store and retrieve > > > trusted public keys. Using `opkg-key` outside a running device is > > > unfeasible as the key folder is hard coded to `/etc/opkg/keys`. > > > > > > This commit adds a variable OPKG_KEYS which defaults to `/etc/opkg/keys` > > > if unset, however allows set arbitrary key folder locations. > > > > > > Arbitrary key folder locations are useful to add signature verification > > > to the ImageBuilders. > > It should be noted that this increases the attack surface. > > > > Especially env variables are easy to set and hard to notice. This could > > allow an attacker to provide a rogue keyring to opkg by just setting an > > environment variable, and this would be quite hard to trace. > > The `opkg` process likely runs as root user, so you have so sneak in env > variables to a root process, which from my understanding root privileges. > > Also the current script uses a relative path to `usign`, which means instead > of providing your own fake keys via OPKG_KEYS, you could just modify the > PATH variable to have a fake version of `usign` running, returning a > successful exit code to anything. > > > I would prefer one of two alternatives: > > > > 1) take a cmdline argument > > Where would that cmdline be added? From the ImageBuilder Makefile? In that > case it'd go through `opkg` to then be passed to `opkg-key`. This would > somewhat force you to use that specific script, while there exists also a > `gnupg` version[1]. > > [1]: https://git.openwrt.org/?p=project/opkg-lede.git;a=blob;f=utils/opkg-key;h=266bb6628a9cae8096f482c4b1d78fe3c3bcb2ca;hb=HEAD > > > 2) provide different opkg-key scripts for device and host usage. Each > > script can use a ~hard-coded path to the expected key location (possibly > > using $TOPDIR etc). > > > > While solution 2) would still use env variables for host usage, it would > > only apply to this host usage of opkg: we do not compromise the security > > of device-based opkg. > > I'm somewhat against maintaining two scripts if not really necessary. If we > go that road we need absolute path for usign, cat, zcat etc. I agree with Paul's arguments and was about to pull this change in. Any objections? > > Paul > > > > Signed-off-by: Paul Spooren <mail@aparcar.org> > > > --- > > > package/system/opkg/files/opkg-key | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/package/system/opkg/files/opkg-key b/package/system/opkg/files/opkg-key > > > index ae5e8a4591..51d1857ad5 100755 > > > --- a/package/system/opkg/files/opkg-key > > > +++ b/package/system/opkg/files/opkg-key > > > @@ -1,5 +1,7 @@ > > > #!/bin/sh > > > +OPKG_KEYS="${OPKG_KEYS:-/etc/opkg/keys}" > > > + > > > usage() { > > > cat <<EOF > > > Usage: $0 <command> <arguments...> > > > @@ -19,7 +21,7 @@ opkg_key_verify() { > > > ( > > > zcat "$msgfile" 2>/dev/null || > > > cat "$msgfile" 2>/dev/null > > > - ) | usign -V -P /etc/opkg/keys -q -x "$sigfile" -m - > > > + ) | usign -V -P "$OPKG_KEYS" -q -x "$sigfile" -m - > > > } > > > opkg_key_add() { > > > @@ -27,8 +29,8 @@ opkg_key_add() { > > > [ -n "$key" ] || usage > > > [ -f "$key" ] || echo "Cannot open file $1" > > > local fingerprint="$(usign -F -p "$key")" > > > - mkdir -p "/etc/opkg/keys" > > > - cp "$key" "/etc/opkg/keys/$fingerprint" > > > + mkdir -p "$OPKG_KEYS" > > > + cp "$key" "$OPKG_KEYS/$fingerprint" > > > } > > > opkg_key_remove() { > > > @@ -36,7 +38,7 @@ opkg_key_remove() { > > > [ -n "$key" ] || usage > > > [ -f "$key" ] || echo "Cannot open file $1" > > > local fingerprint="$(usign -F -p "$key")" > > > - rm -f "/etc/opkg/keys/$fingerprint" > > > + rm -f "$OPKG_KEYS/$fingerprint" > > > } > > > case "$1" in > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Sorry, forgot to reply: On 31-08-20, Daniel Golle wrote: > On Wed, Aug 26, 2020 at 11:57:55AM -1000, Paul Spooren wrote: > > > > On 26.08.20 09:17, Baptiste Jonglez wrote: > > > On 25-08-20, Paul Spooren wrote: > > > > The key folder is used by `opkg` and `usign` to store and retrieve > > > > trusted public keys. Using `opkg-key` outside a running device is > > > > unfeasible as the key folder is hard coded to `/etc/opkg/keys`. > > > > > > > > This commit adds a variable OPKG_KEYS which defaults to `/etc/opkg/keys` > > > > if unset, however allows set arbitrary key folder locations. > > > > > > > > Arbitrary key folder locations are useful to add signature verification > > > > to the ImageBuilders. > > > It should be noted that this increases the attack surface. > > > > > > Especially env variables are easy to set and hard to notice. This could > > > allow an attacker to provide a rogue keyring to opkg by just setting an > > > environment variable, and this would be quite hard to trace. > > > > The `opkg` process likely runs as root user, so you have so sneak in env > > variables to a root process, which from my understanding root privileges. Sure, but most services run as root on OpenWrt. Dnsmasq is a notable exception. > > Also the current script uses a relative path to `usign`, which means instead > > of providing your own fake keys via OPKG_KEYS, you could just modify the > > PATH variable to have a fake version of `usign` running, returning a > > successful exit code to anything. Once somebody finds a way to exploit a network-facing daemon, there are indeed many ways to escalate. My concern is to limit the attack surface if possible, and make it easier to spot any tampering that could have taken place. Spotting a modified PATH is easier than spotting a custom env variable being set. > > > I would prefer one of two alternatives: > > > > > > 1) take a cmdline argument > > > > Where would that cmdline be added? From the ImageBuilder Makefile? In that > > case it'd go through `opkg` to then be passed to `opkg-key`. This would > > somewhat force you to use that specific script, while there exists also a > > `gnupg` version[1]. You're right, it would need further modification in opkg, it's not a good idea. > > [1]: https://git.openwrt.org/?p=project/opkg-lede.git;a=blob;f=utils/opkg-key;h=266bb6628a9cae8096f482c4b1d78fe3c3bcb2ca;hb=HEAD > > > > > 2) provide different opkg-key scripts for device and host usage. Each > > > script can use a ~hard-coded path to the expected key location (possibly > > > using $TOPDIR etc). > > > > > > While solution 2) would still use env variables for host usage, it would > > > only apply to this host usage of opkg: we do not compromise the security > > > of device-based opkg. > > > > I'm somewhat against maintaining two scripts if not really necessary. If we > > go that road we need absolute path for usign, cat, zcat etc. > > I agree with Paul's arguments and was about to pull this change in. > Any objections? While I am still slightly uncomfortable with it, it's not a huge issue (well, I hope so ;) ) and I don't have a better suggestion. So pulling this is fine with me. Baptiste
diff --git a/package/system/opkg/files/opkg-key b/package/system/opkg/files/opkg-key index ae5e8a4591..51d1857ad5 100755 --- a/package/system/opkg/files/opkg-key +++ b/package/system/opkg/files/opkg-key @@ -1,5 +1,7 @@ #!/bin/sh +OPKG_KEYS="${OPKG_KEYS:-/etc/opkg/keys}" + usage() { cat <<EOF Usage: $0 <command> <arguments...> @@ -19,7 +21,7 @@ opkg_key_verify() { ( zcat "$msgfile" 2>/dev/null || cat "$msgfile" 2>/dev/null - ) | usign -V -P /etc/opkg/keys -q -x "$sigfile" -m - + ) | usign -V -P "$OPKG_KEYS" -q -x "$sigfile" -m - } opkg_key_add() { @@ -27,8 +29,8 @@ opkg_key_add() { [ -n "$key" ] || usage [ -f "$key" ] || echo "Cannot open file $1" local fingerprint="$(usign -F -p "$key")" - mkdir -p "/etc/opkg/keys" - cp "$key" "/etc/opkg/keys/$fingerprint" + mkdir -p "$OPKG_KEYS" + cp "$key" "$OPKG_KEYS/$fingerprint" } opkg_key_remove() { @@ -36,7 +38,7 @@ opkg_key_remove() { [ -n "$key" ] || usage [ -f "$key" ] || echo "Cannot open file $1" local fingerprint="$(usign -F -p "$key")" - rm -f "/etc/opkg/keys/$fingerprint" + rm -f "$OPKG_KEYS/$fingerprint" } case "$1" in
The key folder is used by `opkg` and `usign` to store and retrieve trusted public keys. Using `opkg-key` outside a running device is unfeasible as the key folder is hard coded to `/etc/opkg/keys`. This commit adds a variable OPKG_KEYS which defaults to `/etc/opkg/keys` if unset, however allows set arbitrary key folder locations. Arbitrary key folder locations are useful to add signature verification to the ImageBuilders. Signed-off-by: Paul Spooren <mail@aparcar.org> --- package/system/opkg/files/opkg-key | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)