Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

879094 - CVE-2012-5561 - fix permissions on /etc/katello/secure #1349

Merged
merged 4 commits into from Jan 4, 2013

Conversation

Projects
None yet
4 participants
Contributor

jsomara commented Jan 3, 2013

No description provided.

@ghost ghost assigned lzap Jan 3, 2013

Contributor

jsomara commented Jan 3, 2013

thanks to @kseifriedredhat for all of the help! 👍

Contributor

lzap commented Jan 4, 2013

ACK nice find.

@xsuchy xsuchy and 1 other commented on an outdated diff Jan 4, 2013

src/katello.spec
@@ -708,6 +708,10 @@ test -f $TOKEN || (echo $(</dev/urandom tr -dc A-Za-z0-9 | head -c128) > $TOKEN
getent group %{name} >/dev/null || groupadd -r %{name} -g 182
getent passwd %{name} >/dev/null || \
useradd -r -g %{name} -d %{homedir} -u 182 -s /sbin/nologin -c "Katello" %{name}
+# add tomcat & katello to the katello shared group for reading sensitive files
+groupadd katello-shared
@xsuchy

xsuchy Jan 4, 2013

Contributor

This will fail if group already exist. Should be:
getent group katello-shared >/dev/null || groupadd -r %{name}
You may want to allocate static id for katello-shared group (see bz 804204 how to do it), hmmm and why we could not use katello group in first place?

@kseifriedredhat

kseifriedredhat Jan 4, 2013

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/04/2013 02:21 AM, Miroslav Suchý wrote:

In src/katello.spec:

@@ -708,6 +708,10 @@ test -f $TOKEN || (echo $(</dev/urandom tr
-dc A-Za-z0-9 | head -c128) > $TOKEN getent group %{name}

/dev/null || groupadd -r %{name} -g 182 getent passwd %{name}
/dev/null || \ useradd -r -g %{name} -d %{homedir} -u 182 -s
/sbin/nologin -c "Katello" %{name} +# add tomcat & katello to the
katello shared group for reading sensitive files +groupadd
katello-shared

This will fail if group already exist. Should be: getent group
katello-shared >/dev/null || groupadd -r %{name} You may want to
allocate static id for katello-shared group (see bz 804204 how to
do it), hmmm and why we could not use katello group in first
place?

Because more than just katello needs access to this file apparently,
so adding like tomcat to the katello group would be a huge exposure.
Better to minimize the exposure to just the file(s) needed by using a
special group. Longer term my understanding is pulp/candlepin/etc
might need access to this file as well.

— Reply to this email directly or view it on GitHub
https://github.com/Katello/katello/pull/1349/files#r2548308.


Kurt Seifried Red Hat Security Response Team (SRT)
PGP: 0x5E267993 A90B F995 7350 148F 66BF 7554 160D 4553 5E26 7993

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQIcBAEBAgAGBQJQ5qgAAAoJEBYNRVNeJnmTT20QANi3h12OnF864fyCpXDR6xUk
FMXVDAjoQP7DSTgkDvpDggYjI18Y5sKOAQLQlZapfX/5uV/7+e1nD+oGQeY5Uonp
lJBLBehUhHp7Zx7ghbHVoIZlpkVLJYS7vIqmtgNOm19Q4iJHl5x5jb75Y/mK5b41
ehiyUQTmbaDqQhcm1V8BJbLz5OKFB13nW2eqRARLOEpiTgn6OmN+B2PPBBQYxgij
EGDbF1uSBAoYTu2mMSSzfzA4ZFh0BewIGfascn7+SDfGTPcIAvTafq+oepiOgT0v
FffF9l7Bu7V+Ylk+6kUBISbf6yLi9Twh+rZDmZYMKt7dpYd88kF1PdarOBfxScAt
h5YzvG9VwflfHRKoDH2fGQl2UEZHj4NR1cNymNSJThfSVRh7BkMSnvvctbiKC9PT
deM0ap8bRJxQok1WhvsIJe+A6B6drAiAaQO0r5McZzRomxTyPMbA1YRJ2xdT0H7g
Tx5H+GXqVN+tGLLDeAK5NZa5L0H/GEqPkGoclysB5uS1HNuRJoLPlRAvlYjyJSZp
ut4rPYf+5/VZCVFBnGOM7y9UBtUvtcaKMSnBol7aW/R7fyFBunVLKkwDJqZKrznQ
6C30nUNcFUK9t4KAAnJDaIjjpdT+Z9tegSICbpoqokvBgQsTQo/jU6Kr2JVlBImb
2lOVmpuXeKYavrTknozV
=sJ1S
-----END PGP SIGNATURE-----

@xsuchy xsuchy commented on the diff Jan 4, 2013

selinux/katello-selinux/katello-selinux.spec
@@ -94,7 +94,7 @@ install -m 0644 katello-selinux-enable.man8 %{buildroot}%{_mandir}/man8/katello-
install -m 0644 katello-selinux-relabel.man8 %{buildroot}%{_mandir}/man8/katello-selinux-relabel.8
# Install secure (extra protected) directory
-install -d %{buildroot}%{_sysconfdir}/katello/secure
+install -d -m 0750 %{buildroot}%{_sysconfdir}/katello/secure
@xsuchy

xsuchy Jan 4, 2013

Contributor

You should set group ownership in %files section too:
%attr(0755,root,katello-shared) %{_sysconfdir}/katello/secure

@jsomara

jsomara Jan 4, 2013

Contributor

doesn't katello-selinux install first? which could fail if katello-shared is not created

@xsuchy

xsuchy Jan 4, 2013

Contributor

The order is random.
But you can enforce the order by
Requires(pre): katello-common
And I just audited the code and it should be safe.

@xsuchy xsuchy commented on an outdated diff Jan 4, 2013

src/katello.spec
@@ -708,6 +708,10 @@ test -f $TOKEN || (echo $(</dev/urandom tr -dc A-Za-z0-9 | head -c128) > $TOKEN
getent group %{name} >/dev/null || groupadd -r %{name} -g 182
getent passwd %{name} >/dev/null || \
useradd -r -g %{name} -d %{homedir} -u 182 -s /sbin/nologin -c "Katello" %{name}
+# add tomcat & katello to the katello shared group for reading sensitive files
+getent group katello-shared > /dev/null || groupadd -r katello-shared
+usermod -a -G katello-shared tomcat
@xsuchy

xsuchy Jan 4, 2013

Contributor

and user tomcat do not have to exist in this moment.
You should probably move this one line to %post section of headpin and katello (those top packages)
and change
Requires: candlepin-tomcat6
to
Requires(post): candlepin-tomcat6
to make sure tomcat exist in post phase.

Contributor

xsuchy commented Jan 4, 2013

ACK

jsomara added a commit that referenced this pull request Jan 4, 2013

Merge pull request #1349 from jsomara/879094
879094 - CVE-2012-5561 - fix permissions on /etc/katello/secure

@jsomara jsomara merged commit a06b785 into Katello:master Jan 4, 2013

1 check was pending

default The Travis build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment