673116 - Wrong file ownership in debian package

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeremias Bosch (:jbos)]

Attachment: fix maemo fakeroot issue and attach aegis security manifest for maemo 6

Fennecs "make deb" is missing some fakeroot's which cause that the files in the maemo debian package are in ownership and group of the user who is calling make deb.

For Fennec on n900 this is some user 500 in group 500, all files installed on N900 are with that ownership which could cause a security issue. They should be root/root

In MeeGo / Harmattan Context this issue is causing that fennec can not be installed at all, since the "root-user" does not have enough permissions.
Also for MeeGo / Harmattan Fennec is missing the needed aegis security manifest and signing. 

The attached patch fixes those issues.

Comment 1

[Ludovic Hirlimann [:Usul]]

How is this an attack vector ?

Comment 2

[Jeremias Bosch (:jbos)]

Well, maybe i get it wrong but you could simply create such an user "500/500" and alter fennec files in n900,

Comment 3

[Oleg Romashin (:romaxa)]

Comment on attachment 547396 [details] [diff] [review]
fix maemo fakeroot issue and attach aegis security manifest for maemo 6

By some reason Brad did not add this in the beginning, would be nice to know why...

http://hg.mozilla.org/mozilla-central/rev/06b737a2f360

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 547396 [details] [diff] [review]
fix maemo fakeroot issue and attach aegis security manifest for maemo 6

Review of attachment 547396 [details] [diff] [review]:
-----------------------------------------------------------------

no, no reason

Comment 5

[Oleg Romashin (:romaxa)]

I think we need to add small trick here, and make possible to build with maemo6 define on meego platform... please add small check and ignore aegis if that is not available

Comment 6

[Oleg Romashin (:romaxa)]

I think we should add aegis MK define... and check it on configure stage...
please add new version

Comment 7

[Oleg Romashin (:romaxa)]

Comment on attachment 547396 [details] [diff] [review]
fix maemo fakeroot issue and attach aegis security manifest for maemo 6

set this r-, and wait for newer version which does not break non-aegis enabled environment..

Comment 8

[Jeremias Bosch (:jbos)]

Attachment: Check for aegis only in case you want to use harmattan CONTENTMANAGER

I set aegis to contentmanager only as we only need it there to access tracker for the file path.

Comment 9

[Oleg Romashin (:romaxa)]

Comment on attachment 549377 [details] [diff] [review]
Check for aegis only in case you want to use harmattan CONTENTMANAGER


> 		debian/fennec.preinst \
> 		debian/fennec.prerm \
> 		debian/fennec.postinst \
>+		debian/fennec.aegis \
> 		$(NULL)
should not it be also part of define?

Comment 10

[Jeremias Bosch (:jbos)]

Attachment: Check for aegis only in case you want to use harmattan CONTENTMANAGER

updated

Comment 11

[Oleg Romashin (:romaxa)]

Comment on attachment 549809 [details] [diff] [review]
Check for aegis only in case you want to use harmattan CONTENTMANAGER


> 		debian/fennec.preinst \
> 		debian/fennec.prerm \
> 		debian/fennec.postinst \
>+ifdef MOZ_ENABLE_CONTENTMANAGER
>+		debian/fennec.aegis \
>+endif
> 		$(NULL)

I think better style here is:

+ifdef MOZ_ENABLE_CONTENTMANAGER
+PP_DEB_FILES += debian/fennec.aegis \
+ 		$(NULL)
+endif

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)
> Comment on attachment 549809 [details] [diff] [review] [diff] [details] [review]
> Check for aegis only in case you want to use harmattan CONTENTMANAGER
> 
> 
> > 		debian/fennec.preinst \
> > 		debian/fennec.prerm \
> > 		debian/fennec.postinst \
> >+ifdef MOZ_ENABLE_CONTENTMANAGER
> >+		debian/fennec.aegis \
> >+endif
> > 		$(NULL)
> 
> I think better style here is:
> 
> +ifdef MOZ_ENABLE_CONTENTMANAGER
> +PP_DEB_FILES += debian/fennec.aegis \
> + 		$(NULL)
> +endif

Agreed. Please use Oleg's style.

Comment 13

[Jeremias Bosch (:jbos)]

Attachment: nicer style

Comment 14

[Oleg Romashin (:romaxa)]

Comment on attachment 549819 [details] [diff] [review]
nicer style

> MApplicationPage * MeegoFilePicker::createOpenFilePage()
> {
>     QStringList itemType("http://www.semanticdesktop.org/ontologies/2007/03/22/nfo#FileDataObject");
>-    SelectSingleContentItemPage * page = new SelectSingleContentItemPage(QString(),itemType);
>+    SelectSingleContentItemPage * page = new SelectSingleContentItemPage(QString("mp3 mp4 avi acc jpg jpeg png bmp ogg webm"),itemType);
> 
>     connect(page, SIGNAL(contentItemSelected(const QString &)),
>             this, SLOT  (contentItemSelected(const QString &)));
epr... this I guess not related to this patch ... ;)

Comment 15

[Jeremias Bosch (:jbos)]

mhm very odd :O how did this end up here O:-)  ... it should be in the other patch mmmm.

Comment 16

[Jeremias Bosch (:jbos)]

Attachment: This should be it.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 550024 [details] [diff] [review]
This should be it.

>diff -r d89aeb235076 mobile/installer/Makefile.in
>+ifdef MOZ_ENABLE_CONTENTMANAGER
>+PP_DEB_FILES += debian/fennec.aegis \
>+               $(NULL)
>+endif 

I fixed the end-of-line whitespace in the last line of the chunk above, and landed:
  http://hg.mozilla.org/integration/mozilla-inbound/rev/838b538e5b4a

jbos: for future reference, it's best if you can include a checkin message in the patch headers.  In this case, I made one up based on summary / comment 0 / glancing at the patch -- I hope it's accurate enough. :)

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/838b538e5b4a