Feature #7724

Sandbox I2P

Added by intrigeri 2014-08-02 14:27:32 . Updated 2019-03-08 15:55:44 .

Status:
Confirmed
Priority:
Low
Assignee:
Category:
Target version:
Start date:
2015-11-17
Due date:
% Done:

50%

Feature Branch:
kytv:feature/7724-sandbox-i2p
Type of work:
Code
Blueprint:

Starter:
Affected tool:
I2P
Deliverable for:

Description

We want to mitigate the impact of security vulnerabilities that I2P may have. The goals would be:

  • make privilege escalation from the i2psvc user harder
  • make it harder to read identifiers of the local system, user, etc.

Most likely, we’ll want to use AppArmor to do so. Now, it may be hard to confine a Java application in a useful way with AppArmor.

Subtasks

Feature #10572: Peer review of I2P AppArmor profile Confirmed

0
Feature #10573: Fix/remove 'l' permission in I2P apparmor profile Resolved

100
Bug #10924: Remove {,lib/live/mount/overlay/} from the AppArmor profile for I2P Confirmed

0
Bug #10925: I2P is not confined by AppArmor anymore Resolved

100

Related issues

Related to Tails - Feature #5370: AppArmor confinement Resolved 2013-07-27 2014-08-24
Related to Tails - Bug #9949: Audit AppArmor policy vs. hard links Rejected 2015-08-08
Blocked by Tails - Feature #9229: Import I2P 0.9.19 packages Resolved 2015-04-14
Blocked by Tails - Feature #9830: Import I2P 0.9.22 packages Resolved 2015-07-31
Blocked by Tails - Feature #12264: Reintroduce I2P Confirmed 2017-02-25

History

#1 Updated by intrigeri 2014-08-02 14:28:29

  • related to Feature #5385: Have 3 AppArmor profiles in enforce mode added

#2 Updated by intrigeri 2014-08-02 14:28:53

  • blocked by Feature #6198: Have AppArmor support stacked filesystems added

#3 Updated by intrigeri 2014-10-05 06:26:40

  • blocks deleted (Feature #6198: Have AppArmor support stacked filesystems)

#4 Updated by intrigeri 2014-10-05 06:26:43

  • related to deleted (Feature #5385: Have 3 AppArmor profiles in enforce mode)

#5 Updated by intrigeri 2014-10-05 06:27:20

#6 Updated by sajolida 2014-11-03 20:58:53

  • Target version set to Hardening_M1

#7 Updated by kytv 2015-02-02 17:59:44

  • Status changed from Confirmed to In Progress

I’m working on this upstream and think that the 0.9.18 packages will address this ticket.

#8 Updated by kytv 2015-02-23 03:39:50

#9 Updated by kytv 2015-02-23 03:47:08

The I2P daemon is confined by AppArmor in the 0.9.18 packages, which I think will satisfy this ticket’s requirements.

#10 Updated by kytv 2015-02-23 03:47:25

  • Target version changed from Hardening_M1 to Tails_1.3.2

#11 Updated by intrigeri 2015-02-23 09:44:00

> The I2P daemon is confined by AppArmor in the 0.9.18 packages, which I think will satisfy this ticket’s requirements.

Yay! At first glance, this doesn’t satisfy the criteria for inclusion in a point-release, but I’ll let the RM judge.

Were these AppArmor profiles reviewed by someone skilled at AppArmor?

#12 Updated by kytv 2015-02-23 12:59:22

intrigeri wrote:
> > The I2P daemon is confined by AppArmor in the 0.9.18 packages, which I think will satisfy this ticket’s requirements.
>
> Yay! At first glance, this doesn’t satisfy the criteria for inclusion in a point-release, but I’ll let the RM judge.

Valid point.

> Were these AppArmor profiles reviewed by someone skilled at AppArmor?

Not yet.

I started in a VM in enforce-mode disallowing everything then gradually adding rules to make it work. Then I added deny rules to avoid spamming the logs with denials for accesses that just aren’t needed. I’ve been using the apparmor rules in enforce mode for a few weeks (in and outside of Tails) and everything still works. As far as functionality goes the rules are permissive enough.

On the other hand, another set of eyes would be most welcome to ensure that the rules I’ve defined are strict enough to be useful and whether certain abstractions should be avoided.

#13 Updated by kytv 2015-04-06 15:18:39

  • Target version changed from Tails_1.3.2 to Tails_1.4
  • QA Check set to Ready for QA

The I2P 0.9.18 packages come with an a apparmor profile that I created and these packages were included in Tails 1.3.1. Since Tails 1.3.1, I2P has been confined with apparmor in enforcing mode.

The profiles have not been reviewed by anyone skilled with apparmor and perhaps this is needed before this ticket can be closed.

#14 Updated by kytv 2015-04-06 15:18:59

  • Assignee deleted (kytv)

#15 Updated by intrigeri 2015-04-06 15:39:10

  • Assignee set to intrigeri

#16 Updated by intrigeri 2015-04-09 11:45:05

  • Assignee changed from intrigeri to kytv
  • % Done changed from 0 to 30
  • QA Check changed from Ready for QA to Dev Needed
  • Is there a Vcs-Git somewhere for the packaging? This would make the next dev/review cycles easier for me.
  • Please follow the indentation best practices as seen in /etc/apparmor.d/abstractions/*.
  • It’s not clear what the usr.bin.i2prouter profile is for:
    • the initscript explicitly runs the wrapper with the system_i2p profile
    • in Tails 1.3.2, with I2P enabled and running, I see no process confined by usr.bin.i2prouter, so I wonder what it’s about, and whether I should review it or not.
  • Do we really need to give access to {PROC}/[0-9]*/net/if_inet6 and {PROC}/[0-9]*/net/ipv6_route. IOW, what happens if we deny access to these files?
  • Perhaps /usr/lib/jvm/java-*-openjdk-*/jre/lib/i386/client/classes.jsa should be tuned to support more than one arch?
  • The i2p abstraction has /etc/fonts/**, which is covered by the fonts abstraction, that you include already. I suspect there may be other similar cases, so please double check every line for similar/same ones in abstractions.
  • Does the i2p service really need to read /etc/default/i2p? Usually, only the initscript itself does.
  • Perhaps prepending owner to the PROC thingies would work just as well, and harden things a bit?

Otherwise, looks pretty good! Once we’ve completed our internal review, if you feel like it, you can also ask for a review on the AppArmor mailing-list. They’re nice people, and more experienced than I am.

#17 Updated by kytv 2015-04-09 19:47:33

intrigeri wrote:
> * Is there a Vcs-Git somewhere for the packaging? This would make the next dev/review cycles easier for me.

I2P is managed with Monotone but I export the source to https://github.com/i2p/i2p.i2p, so this might be useful for future reviews. The git repo I use for my own packaging bits isn’t online. Maybe after some filter-branch action (to remove identifying info) I can start publishing that. I do keep ./debian synched, however, with the upstream I2P repo.

> * Please follow the indentation best practices as seen in /etc/apparmor.d/abstractions/*.

OK.

> * It’s not clear what the usr.bin.i2prouter profile is for:
> the initscript explicitly runs the wrapper with the system_i2p profile
> in Tails 1.3.2, with I2P enabled and running, I see no process confined by usr.bin.i2prouter, so I wonder what it’s about, and whether I should review it or not.

It’s not used by Tails. Some users don’t want to have it run as a daemon, instead they want to use i2prouter start as they would without using the packaged version.

(Side note: Since we don’t use /usr/bin/i2prouter in Tails, should it and its associated apparmor rules be removed during the build? If so I can create a new ticket and merge request.)

> * Do we really need to give access to {PROC}/[0-9]*/net/if_inet6 and {PROC}/[0-9]*/net/ipv6_route. IOW, what happens if we deny access to these files?

I suppose this could be disabled during the Tails build.

> * Perhaps /usr/lib/jvm/java-*-openjdk-*/jre/lib/i386/client/classes.jsa should be tuned to support more than one arch?

This is actually only used by 32-bit JVMs. I only have the following on my amd64 box (which isn’t needed by I2P):

/usr/lib/jvm/java-6-openjdk-amd64/jre/lib/amd64/server/classes.jsa
/usr/lib/jvm/java-7-openjdk-amd64/jre/lib/amd64/server/classes.jsa
/usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server/classes.jsa

> * The i2p abstraction has /etc/fonts/**, which is covered by the fonts abstraction, that you include already. I suspect there may be other similar cases, so please double check every line for similar/same ones in abstractions.

OK.

> * Does the i2p service really need to read /etc/default/i2p? Usually, only the initscript itself does.

To be confirmed but I suspect you’re correct that it’s not required.

> * Perhaps prepending owner to the PROC thingies would work just as well, and harden things a bit?

To be confirmed, but probably so.

> Otherwise, looks pretty good!

Thanks. :) (As I think I’ve said already, this was my first time “dipping into” the world of apparmor profiles.)

> Once we’ve completed our internal review, if you feel like it, you can also ask for a review on the AppArmor mailing-list. They’re nice people, and more experienced than I am.

#18 Updated by kytv 2015-04-14 01:11:27

kytv wrote:
> intrigeri wrote:

> > * Do we really need to give access to {PROC}/[0-9]*/net/if_inet6 and {PROC}/[0-9]*/net/ipv6_route. IOW, what happens if we deny access to these files?
>
> I suppose this could be disabled during the Tails build.

I disabled this on my system and I’m still communicating over IPv6. I removed this from the 0.9.19 packages as it’s not needed.

Additionally:

  • Access to /etc/default/i2p wasn’t needed. Removed. (now that I’m loading the profile from the initscript I can’t remember what I was doing before but this was needed then…)
  • Prepending owner to the @{PROC} lines worked fine.
  • Indentation was done according to files found in apparmor.d/abstractions

The only things left would be dealing with the usr.bin.i2prouter rule (if wanted) (and possibly /usr/bin/i2prouter) bits.

#19 Updated by kytv 2015-04-14 01:14:25

#20 Updated by kytv 2015-04-14 01:14:30

#21 Updated by kytv 2015-04-14 20:38:31

kytv wrote:
> kytv wrote:
> > intrigeri wrote:
>
> > > * Do we really need to give access to {PROC}/[0-9]*/net/if_inet6 and {PROC}/[0-9]*/net/ipv6_route. IOW, what happens if we deny access to these files?
> >
> > I suppose this could be disabled during the Tails build.
>
> I disabled this on my system and I’m still communicating over IPv6. I removed this from the 0.9.19 packages as it’s not needed.

I was mistaken. This is actually needed for the upstream packages but this could be disabled for Tails.

#22 Updated by kytv 2015-04-22 12:47:25

  • Assignee changed from kytv to intrigeri
  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch set to kytv:feature/7724-sandbox-i2p

In this branch I

  1. disable IPv6 in Apparmor
  2. remove the i2prouter apparmor rules and replace /usr/bin/i2prouter with a tiny stub script which outputs information that the script is unused in Tails.

I think the other concerns from Feature #7724#note-16 were addressed in the upstream package.

#23 Updated by intrigeri 2015-04-26 04:21:14

  • Assignee changed from intrigeri to kytv
  • QA Check changed from Ready for QA to Dev Needed

kytv wrote:
> disable IPv6 in Apparmor

Just a bit of nitpicking: I would prefer if the deny rules that replace the 2 upstream ones were put at the same place, instead of being re-introduced 40+ lines below. This would make the diff a tiny bit more readable IMO. Your call: if you disagree, I won’t insist :)

> remove the i2prouter apparmor rules and replace /usr/bin/i2prouter with a tiny stub script which outputs information that the script is unused in Tails.

Please use dpkg-divert for the three deleted or modified files, so that we’re idempotent wrt. APT upgrades. There are examples in config/chroot_local-hooks/.

> I think the other concerns from Feature #7724#note-16 were addressed in the upstream package.

Great! I’ll take a look once my Git clone completes + time allows.

#24 Updated by intrigeri 2015-04-27 01:53:26

  • % Done changed from 30 to 50

Better! It’s getting pretty good. I reviewed the current state as found in the 0.9.19-3~deb7u+1 package in the Tails APT repo.

  • Thanks for the pointer to the Git repo. In the future, please do atomic commits with more detailed commit messages (at least in debian/apparmor), to ease reviews :) I’m sure you’ll understand that things like commit 11c3230 are a pain to review.
  • The Git repo only has 0.9.18-1 so far. Forgot to sync?
  • A few (#include and network) lines still aren’t indented correctly.
  • I know we don’t care about it for Tails yet, but perhaps the 2 IPv6-related PROC rules could be prefixed by owner as well?
  • Did you “double check every line for similar/same ones in abstractions”? E.g. the /dev/random rule is covered by the base abstraction on current sid (didn’t check on Wheezy, sorry).
  • Can’t /sys/devices/system/cpu/** be made a little bit tighter? There are a lot of files in there.
  • Do we really need the user-tmp abstraction? At first glance, it seems to me that we’re overriding most of this abstraction rules.
  • Can owner /{,var/}tmp/** be made tighter? Isn’t there any prefix shared by temporary files created in there by I2P stuff. If not, no big deal.
  • “Used by some versions of the Tanuki wrapper, not needed by I2P” is confusing in a profile that’s about I2P. I trust you that the two following rules are needed, but please clarify the comment :)
  • Unrelated to Tails, but I care about it so that people around don’t mentally associate AppArmor with “breaks stuff”: please consider testing these profiles under Ubuntu (latest LTS and current non-LTS) — Ubuntu has additional AppArmor kernel features that may break things if profiles don’t support them. If you can’t be bothered doing it, just tell me and I’ll do it myself when time allows.

#25 Updated by kytv 2015-05-03 08:35:46

  • Target version changed from Tails_1.4 to Tails_1.5

#26 Updated by kytv 2015-05-21 18:00:04

intrigeri wrote:
> Better! It’s getting pretty good. I reviewed the current state as found in the 0.9.19-3~deb7u+1 package in the Tails APT repo.
>
> * Thanks for the pointer to the Git repo. In the future, please do atomic commits with more detailed commit messages (at least in debian/apparmor), to ease reviews :) I’m sure you’ll understand that things like commit 11c3230 are a pain to review.

OK.

> * The Git repo only has 0.9.18-1 so far. Forgot to sync?

I neglected to check in the changelog file. (I should try going more of my packaging work in monotone, I just like Git and git-buildpackage too much :S)

> * A few (#include and network) lines still aren’t indented correctly.

Oops. I think those are good now.

> * I know we don’t care about it for Tails yet, but perhaps the 2 IPv6-related PROC rules could be prefixed by owner as well?

Those are owned by root so the i2psvc user would be denied access.

> * Did you “double check every line for similar/same ones in abstractions”? E.g. the /dev/random rule is covered by the base abstraction on current sid (didn’t check on Wheezy, sorry).

I’ll be sure to do that. (I can confirm that (u)?random are indeed in the base abstraction, even in Wheezy.

> * Can’t /sys/devices/system/cpu/** be made a little bit tighter? There are a lot of files in there.

Maybe.

> * Do we really need the user-tmp abstraction? At first glance, it seems to me that we’re overriding most of this abstraction rules.

TBD.

> * Can owner /{,var/}tmp/** be made tighter? Isn’t there any prefix shared by temporary files created in there by I2P stuff. If not, no big deal.

TBD.

> * “Used by some versions of the Tanuki wrapper, not needed by I2P” is confusing in a profile that’s about I2P. I trust you that the two following rules are needed, but please clarify the comment :)

It should be clearer now.

> * Unrelated to Tails, but I care about it so that people around don’t mentally associate AppArmor with “breaks stuff”: please consider testing these profiles under Ubuntu (latest LTS and current non-LTS) — Ubuntu has additional AppArmor kernel features that may break things if profiles don’t support them. If you can’t be bothered doing it, just tell me and I’ll do it myself when time allows.

I’ll be sure to do that. (I did check some versions of Ubuntu but I don’t remember which those were. I know that’s so very helpful. :s)

#27 Updated by intrigeri 2015-05-21 18:48:00

Great to see progress here! :)

#28 Updated by kytv 2015-08-02 07:49:24

  • Assignee changed from kytv to intrigeri
  • Target version changed from Tails_1.5 to Tails_1.6
  • QA Check changed from Dev Needed to Ready for QA

Changes pushed, dependent on the 0.9.21 packages.

#29 Updated by kytv 2015-08-02 07:49:42

#30 Updated by kytv 2015-08-02 07:55:34

  • Assignee changed from intrigeri to kytv

Actually, I want to look over the apparmor rules one more time. Taking this one back for now.

#31 Updated by intrigeri 2015-08-02 09:15:26

> Actually, I want to look over the apparmor rules one more time. Taking this one back for now.

OK. Please also ask for a review on the AppArmor mailing-list, before submitting it for review to me.

#32 Updated by intrigeri 2015-08-08 03:00:33

  • related to Bug #9949: Audit AppArmor policy vs. hard links added

#33 Updated by intrigeri 2015-08-08 03:06:00

  • QA Check changed from Ready for QA to Dev Needed

See Bug #9949#note-3: the “l” permission in the i2p abstraction makes the rest of the policy basically useless, at least wrt. access to non-persistent files stored on the root filesystem.

#34 Updated by kytv 2015-09-20 15:39:38

  • Target version changed from Tails_1.6 to Tails_1.7

#35 Updated by intrigeri 2015-09-21 02:04:56

This ticket has been postponed 4 times in 6 months. I suggest creating subtasks for the main remaining issues (iirc: the l permission and lack of peer review), which should help setting a realistic timeline for the whole thing. [Yeah, I know, I have some tickets in the same situation myself..]

#36 Updated by kytv 2015-10-05 02:32:33

  • QA Check changed from Dev Needed to Ready for QA

#37 Updated by kytv 2015-10-05 02:36:02

  • QA Check changed from Ready for QA to Dev Needed

#38 Updated by kytv 2015-11-17 15:43:55

  • Target version changed from Tails_1.7 to 246

#39 Updated by sajolida 2015-11-27 04:46:24

  • Target version changed from 246 to Tails_2.0

#40 Updated by kytv 2015-11-28 18:47:22

  • Target version changed from Tails_2.0 to Tails_2.2

Likely not before 2.0

#41 Updated by intrigeri 2016-01-13 14:01:50

The “feature branch” is not relevant anymore, is it?

#42 Updated by intrigeri 2016-01-18 14:45:00

kytv: given how this was handled in the last 6 months, I humbly suggest that you consider calling for help regarding the maintenance of I2P in Tails. -dev@ would be a good start, and we can relay this via Twitter.

#43 Updated by anonym 2016-03-29 18:13:32

  • Target version changed from Tails_2.2 to Tails_2.3

#44 Updated by anonym 2016-05-08 05:10:25

  • Target version changed from Tails_2.3 to Tails_2.4

#45 Updated by anonym 2016-06-08 01:34:53

  • Target version changed from Tails_2.4 to Tails_2.5

#46 Updated by BitingBird 2016-07-01 11:28:52

  • Assignee deleted (kytv)
  • Target version deleted (Tails_2.5)

no news from kytv -> removing assignee and target version

#47 Updated by Anonymous 2017-06-29 09:19:09

We don’t ship i2p anymore atm. Lowering priority.

#48 Updated by Anonymous 2018-01-15 11:38:58

#49 Updated by intrigeri 2019-03-08 15:55:44

  • Status changed from In Progress to Confirmed