Tails

#5 Updated by anonym 2017-09-22 14:39:06

intrigeri wrote:
> The changes look good. I’m confused by ++ deny {HOME}/.mozilla/firefox/bookmarks/ r,@: that’s the place where we store our bookmarks, so it feels a bit weird that Tor Browser does not really need read access there. But apparently this works, so: fine :)

I did test it, of course! :)

My reasoning why this works: previously this directory (and operation on it) was implicitly disallowed since no rule matches it; we only explicitly grant rights to the bookmarks via .../bookmarks/** rwk because Firefox knows which files to open and don’t have to list the directory (even though it tries). So explicitly disallowing this directory changes nothing except dropping the audit log, which is what we want. Makes sense?

> Process-wise:
>
> * I see no corresponding branch in our torbrowser-launcher.git => your work will be lost, or my work will be painful, next time I do something like Bug #14602.
> * I see no upstream pull request while these changes don’t seem to be Tails-specific. Now that I’ve reviewed them, please go ahead :)
> * Ideally you would prepare a branch forked off upstream master, with the changes submitted upstream, and then merge it into a feature/14606-silence-tor-browser-apparmor-logs branch forked off our own master branch. I’ll merge the latter once tested and happy.

Ack! I’ll to document this in wiki/src/contribute/release_process/browser-apparmor-patch.mdwn in the same branch so I hopefully won’t do this mistake again.

#6 Updated by anonym 2017-09-22 14:45:31

intrigeri wrote:
> * I see no upstream pull request while these changes don’t seem to be Tails-specific. Now that I’ve reviewed them, please go ahead :)

Well, the rule for @{HOME}/.mozilla/firefox/bookmarks/ is Tails-specific, so I’ll drop that one.

#8 Updated by intrigeri 2017-09-23 16:02:48

> intrigeri wrote:
>> The changes look good. I’m confused by ++ deny {HOME}/.mozilla/firefox/bookmarks/ r,@: that’s the place where we store our bookmarks, so it feels a bit weird that Tor Browser does not really need read access there. But apparently this works, so: fine :)

> I did test it, of course! :)

> My reasoning why this works: previously this directory (and operation on it) was implicitly disallowed since no rule matches it; we only explicitly grant rights to the bookmarks via .../bookmarks/** rwk because Firefox knows which files to open and don’t have to list the directory (even though it tries). So explicitly disallowing this directory changes nothing except dropping the audit log, which is what we want. Makes sense?

OK. (I’m always confused by apparmor.d(5) and http://wiki.apparmor.net/index.php/AppArmor_Core_Policy_Reference#AppArmor_globbing_syntax that both say that ** can match 0 or more chars, so at first glance I would assume that .../bookmarks/** rwk also applies to .../bookmarks/. Now, I had to specify permissions for the directory itself separately in many profiles, so that’s just a doc problem and your reasoning makes sense!)

>> Process-wise:
> […]
> Ack! I’ll to document this in wiki/src/contribute/release_process/browser-apparmor-patch.mdwn in the same branch so I hopefully won’t do this mistake again.

Nice! Some comments about it:

• Please rename “browser-apparmor-patch.mdwn” to “tor-browser_AppArmor_patch.mdwn” for consistency + to have nicer links and URLs (ikiwiki treats underscores as space).
• I think the “Figure our which is the most recent Debian release” instructions lack some kind of git fetch. Also, looking at the Debian packaging tags might be easier and more reliable than looking at the changelog.
• Calling a remote upstream feels risky: many Debian packaging practices use “upstream” as a branch namespace. For this reason, the common practice is to call the upstream remote upstream-repo instead.

Looks good otherwise :)

#9 Updated by intrigeri 2017-09-23 16:04:35

> Ack! I’ll to document this in wiki/src/contribute/release_process/browser-apparmor-patch.mdwn in the same branch so I hopefully won’t do this mistake again.

One last thing about this doc: it doesn’t explain how one applies such changes to tails.git. Not a blocker of course, but if you have time you could do that and ensure the result of following that new doc matches what you already pushed to tails.git using a different technique :)

#10 Updated by intrigeri 2017-09-23 16:24:13

> I actually realized that my previous rules wouldn’t catch the case if the directories it deny:s access to exist. Fixed!

:)

> In the torbrowser-launcher repo there’s the silence-tor-browser-apparmor-logs branch which is intended as the upstream pull request. I’ll send it once you’re ok with the changes. Perhaps the docs should include that, i.e. only submit the PR once it’s merged in Tails (=> has been reviewed)?

The approach you’re proposing ensures 2 pair of eyes have seen the changes before we submit upstream. OTOH it’s a recipe to forget to actually submit the changes upstream (because it won’t be tracked by any ticket anymore, unless we don’t mark the ticket “fix committed” unless the PR has been sent). Let’s try that and we’ll see. Another option would be that the reviewer submits the PR once happy, which might give us the best of both worlds?

Code review passes but I’m a bit lost: we’re not addressing the specific audit logs this ticket was created for here. I’ll manually test the “Check for updates” button before merging; this should cover all kinds of update checks (after glancing over the tickets that lead to create this ticket, it’s unclear to me if our disabling via prefs of automatic check for updates still works, so to be safe I’ll assume Tor Browser may eventually do that on its own, and the fastest way to see what would happen is probably to check for updates manually).

#12 Updated by intrigeri 2017-09-24 05:13:02

> Code review passes but I’m a bit lost: we’re not addressing the specific audit logs this ticket was created for here. I’ll manually test the “Check for updates” button before merging; this should cover all kinds of update checks […]

Opening the About dialog (without clicking the “Check for updates” button) triggers this:

AVC apparmor="DENIED" operation="mkdir" profile="/usr/local/lib/tor-browser/firefox" name="/usr/local/lib/tor-browser/TorBrowser/UpdateInfo/" pid=11108 comm="firefox" requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000

Restarting Tor Browser and waiting a bit produces:

apparmor="DENIED" operation="mkdir" profile="/usr/local/lib/tor-browser/firefox" name="/usr/local/lib/tor-browser/update.test/" pid=2692 comm="firefox" requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000

⇒ the issue this ticket was originally about still occurs.

(Also, if I activate the Screen Reader and then start Tor Browser, I see:

AVC apparmor="DENIED" operation="mkdir" profile="/usr/local/lib/tor-browser/firefox" name="/run/user/1000/at-spi2-ED146Y/" pid=2507 comm="firefox" requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000

… but the Screen Reader seems to work. Same for the Screen Keyboard. I did not test these extensively enough so I’m not sure if not having access to that directory has any problematic consequence ⇒ I would not classify this as noise we should silence without further investigation ⇒ off-topic here for now.)

#13 Updated by anonym 2017-09-24 21:52:32

intrigeri wrote:
> > Code review passes but I’m a bit lost: we’re not addressing the specific audit logs this ticket was created for here. I’ll manually test the “Check for updates” button before merging; this should cover all kinds of update checks […]
>
> Opening the About dialog (without clicking the “Check for updates” button) triggers this:
>
> […]
>
> Restarting Tor Browser and waiting a bit produces:
>
> […]
>
> ⇒ the issue this ticket was originally about still occurs.

tl;dr: I’ll fix it!

quite offtopic>

I still have the VM that I tested all this in running, and something incredibly strange is going on.

Background: I was running this command to monitor for those update entries:

journalctl --lines=all --all --follow | grep "kernel: audit:" | grep browser

and after at least an hour I only got:

Sep 24 21:02:33 localhost.localdomain kernel: audit: type=1400 audit(1506286953.956:5): apparmor="STATUS" operation="profile_load" name="/usr/local/lib/tor-browser/firefox" pid=601 comm="apparmor_parser"

so I felt confident that my command was correct, and that apparently the automatic update check was properly disabled. (I just re-ran the command (actually that is what I copy-pasted above) now, more than 24h later, without seeing any entry about the update check.) As long as the automatic check was disabled I thought it would be a good idea to not ignore these entries so we’d see when our prefs fail to disable it next time (and we don’t really care about the case when users manually click the check button).

Now for the strange thing: when I run this command (i.e. the above command without the last | grep):

journalctl --lines=all --all --follow | grep "kernel: audit:"

then I get lots more lines, as expected, but I also get:

[...]
Sep 24 21:08:50 amnesia kernel: audit: type=1400 audit(1506287330.530:34): apparmor="DENIED" operation="mkdir" profile="/usr/local/lib/tor-browser/firefox" name="/usr/local/lib/tor-browser/TorBrowser/UpdateInfo/" pid=9171 comm="firefox" requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000
Sep 24 21:08:50 amnesia kernel: audit: type=1400 audit(1506287330.530:35): apparmor="DENIED" operation="mkdir" profile="/usr/local/lib/tor-browser/firefox" name="/usr/local/lib/tor-browser/update.test/" pid=9171 comm="firefox" requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000
[...]

I mean, that “extra” grep matches both of those lines, so why are they not printed by the first command? In fact, I can even do | cat instead, which should be a no-op, but that also hides these entries. WTF? I am extremely confused, but prepared to be pointed out the obvious thing I have missed…

/quite offtopic>

> (Also, if I activate the Screen Reader and then start Tor Browser, I see:
>
> […]
>
> … but the Screen Reader seems to work. Same for the Screen Keyboard. I did not test these extensively enough so I’m not sure if not having access to that directory has any problematic consequence ⇒ I would not classify this as noise we should silence without further investigation ⇒ off-topic here for now.)

Agreed!

#14 Updated by anonym 2017-09-24 22:02:19

anonym wrote:
> quite offtopic>
> […]
> /quite offtopic>

I could reproduce in a 3.2~rc1 VM, and then you get:

journalctl --lines=all --all --follow | grep "kernel: audit:" | grep browser
Sep 24 21:54:53 localhost.localdomain kernel: audit: type=1400 audit(1506290093.081:5): apparmor="STATUS" operation="profile_load" name="/usr/local/lib/tor-browser/firefox" pid=741 comm="apparmor_parser"
Sep 24 21:56:20 amnesia kernel: audit: type=1400 audit(1506290180.605:29): apparmor="DENIED" operation="mkdir" profile="/usr/local/lib/tor-browser/firefox" name="/home/amnesia/.cache/fontconfig/" pid=10999 comm="firefox" requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000

and

journalctl --lines=all --all --follow | grep "kernel: audit:"    
[... ignoring lines without "browser" ...]
Sep 24 21:54:53 localhost.localdomain kernel: audit: type=1400 audit(1506290093.081:5): apparmor="STATUS" operation="profile_load" name="/usr/local/lib/tor-browser/firefox" pid=741 comm="apparmor_parser"
[... ignoring lines without "browser" ...]
Sep 24 21:56:20 amnesia kernel: audit: type=1400 audit(1506290180.605:29): apparmor="DENIED" operation="mkdir" profile="/usr/local/lib/tor-browser/firefox" name="/home/amnesia/.cache/fontconfig/" pid=10999 comm="firefox" requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000
Sep 24 21:56:23 amnesia kernel: audit: type=1400 audit(1506290183.900:30): apparmor="DENIED" operation="mkdir" profile="/usr/local/lib/tor-browser/firefox" name="/home/amnesia/.cache/fontconfig/" pid=10999 comm="firefox" requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000
Sep 24 21:56:24 amnesia kernel: audit: type=1400 audit(1506290184.176:31): apparmor="DENIED" operation="open" profile="/usr/local/lib/tor-browser/firefox" name="/home/amnesia/.mozilla/firefox/bookmarks/" pid=10999 comm="firefox" requested_mask="r" denied_mask="r" fsuid=1000 ouid=1000
Sep 24 21:56:28 amnesia kernel: audit: type=1400 audit(1506290188.935:32): apparmor="DENIED" operation="mkdir" profile="/usr/local/lib/tor-browser/firefox" name="/home/amnesia/.cache/fontconfig/" pid=11092 comm=57656220436F6E74656E74 requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000
Sep 24 21:56:30 amnesia kernel: audit: type=1400 audit(1506290190.009:33): apparmor="DENIED" operation="mkdir" profile="/usr/local/lib/tor-browser/firefox" name="/home/amnesia/.cache/fontconfig/" pid=11092 comm=57656220436F6E74656E74 requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000

so the strange thing is still there, and now I am even more curious about what is going on.

#15 Updated by anonym 2017-09-24 22:06:06

Gah, the answer to my mystery: grep --line-buffered. Composing multiple grep:s with pipes is apparently a bad idea without that option.

#20 Updated by anonym 2017-09-25 14:48:44

intrigeri wrote:
> Hi!
>
> > intrigeri wrote:
> >> Also, looking at the Debian packaging tags might be easier and more reliable than looking at the changelog.
>
> > Ack, I fully automated that part now.
>
> Fancy! I didn’t know --sort=version:refname, glad to learn about it :)

man git-tag taught it to me yesterday!

> Note: this won’t work as expected if there’s a newer torbrowser-launcher in experimental that updates the AppArmor profile we’re patching, but that’s fine I think. Bonus points if you add a note in the doc about it, but I certainly won’t block on it.

Added!

> >> One last thing about this doc: it doesn’t explain how one applies such changes to tails.git. Not a blocker of course, but if you have time you could do that and ensure the result of following that new doc matches what you already pushed to tails.git using a different technique :)
>
> > I just opened the patch in Tails’ Git in emacs and manually copy-pasted the lines in there, while relying on emacs’ Diff mode to keep the hunk numbers up-to-date. I’m a bit at loss of how to do this properly. What about generating a Git patch from the commit in the torbrowser-launcher repo, changing the path of the patch so that it’s the same as its location in Tails’ Git, then import the patch with git am and deal with potential merge conflicts vs our diversions? Or do you have a better proposal?
>
> I dislike “generating a Git patch from the commit” […]

Agreed.

However, to me it seems whatever instructions I would come up with would be complex and confusing, so I’ll leave this as an exercise to the reader.

> >> > In the torbrowser-launcher repo there’s the silence-tor-browser-apparmor-logs branch which is intended as the upstream pull request. I’ll send it once you’re ok with the changes. Perhaps the docs should include that, i.e. only submit the PR once it’s merged in Tails (=> has been reviewed)?
> >> The approach you’re proposing ensures 2 pair of eyes have seen the changes before we submit upstream. OTOH it’s a recipe to forget to actually submit the changes upstream (because it won’t be tracked by any ticket anymore, unless we don’t mark the ticket “fix committed” unless the PR has been sent). Let’s try that and we’ll see. Another option would be that the reviewer submits the PR once happy, which might give us the best of both worlds?
>
> > Sounds good! I added it to the docs.
>
> Looks OK modulo typo in “Then submit and submit”?
>
> > I’ve also pushed an updated patch (our only, since it doesn’t make sense upstream) that ignore the update check related operations.
>
> Looks OK, although it would be nicer if moved after deny {PROC}/{pid}/net/route r, to preserve sorting.

When looking elsewhere in that profile, it seems the sorting I applied is the one used, i.e. treat ${PROC}/x as “p” (not “x”) and ${HOME}/y as “h” (not “y”).

> I can’t find your updates in torbrowser-launcher.git so I can’t possibly merge them. Forgot to push?

No, they were already pushed, so I’m not sure what’s up on your side.

Even though you didn’t explicitly ack my last change (deny update check directories) to torbrowser-AppArmor-profile.patch in Tails’ Git, I’ll merge this for Tails 3.2; I feel confident doing so since this branch doesn’t introduce any regressions in any tests done so far.

I’ll leave this ticket open while we sort out the upstreaming bits.