Feature #7873

Audit OnionShare

Added by Anonymous 2014-09-03 13:11:03 . Updated 2016-03-21 09:59:56 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Target version:
Start date:
Due date:
% Done:

100%

Feature Branch:
Type of work:
Security Audit
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

Before including it into Tails..


Files

bandit.txt (5689 B) hybridwipe, 2015-09-07 16:06:25

Subtasks


Related issues

Blocks Tails - Feature #7870: Include OnionShare Resolved 2016-12-07
Blocked by Tails - Bug #8464: Wait for problems identified in initial audit of OnionShare to be fixed upstream Resolved 2014-12-19

History

#1 Updated by Anonymous 2014-09-03 13:11:35

Before/if.

#2 Updated by Anonymous 2014-09-03 13:16:21

#3 Updated by Anonymous 2014-09-03 13:16:41

#4 Updated by sajolida 2014-09-04 04:17:09

  • Tracker changed from Bug to Feature
  • Status changed from New to Confirmed

#5 Updated by jvoisin 2014-12-10 00:10:16

  • % Done changed from 0 to 20

I took a quick glance at the code, and sent some comments to micah.
It looks great so far.

#6 Updated by BitingBird 2014-12-10 00:37:47

  • Status changed from Confirmed to In Progress

#7 Updated by Anonymous 2014-12-10 19:47:10

<333

#8 Updated by jvoisin 2014-12-18 01:06:54

I finished a quick audit, and I would recommend to not include OnionShare into Tails, for now.
But, I’ll be happy to audit it (more in depth) again once some worrying issues are fixed, and a clear design documented drafted/published.

I’d like to emphasize the fact that this software is really well written, with safety in mind.

#9 Updated by Anonymous 2014-12-18 12:53:50

  • Priority changed from Normal to Low

Thanks for doing this!

Will you keep track of changes in the code base?

I am changing the priority of this ticket to low until something happens on the upstream side of things.

#10 Updated by intrigeri 2014-12-19 10:47:49

> I am changing the priority of this ticket to low until something happens on the upstream side of things.

I don’t think that low priority means what you want here. We have way better means to indicate that we’re waiting for something to happen, e.g. create a subtask called “Wait for problems identified in initial audit of OnionShare to be fixed upstream”, with type of work = Wait.

#11 Updated by Anonymous 2014-12-19 11:49:56

  • blocked by Bug #8464: Wait for problems identified in initial audit of OnionShare to be fixed upstream added

#12 Updated by Anonymous 2014-12-19 11:50:11

  • Priority changed from Low to Normal

#13 Updated by BitingBird 2015-01-03 02:39:06

  • Assignee deleted (jvoisin)

#14 Updated by BitingBird 2015-04-10 18:27:47

Code hasn’t been updated yet.

#15 Updated by anarcat 2015-05-14 15:29:04

jvoisin wrote:
> I finished a quick audit, and I would recommend to not include OnionShare into Tails, for now.
> But, I’ll be happy to audit it (more in depth) again once some worrying issues are fixed, and a clear design documented drafted/published.
>
> I’d like to emphasize the fact that this software is really well written, with safety in mind.

This was 5 months ago. I don’t see anything in the upstream issue tracker, nor any details here about specific problems with onionshare, which is already available in Debian.

Why isn’t it part of tails? It seems like useful software.

I think responsible disclosure here implies sharing those issues with the public at some point, if upstream doesn’t do anything (assuming that upstream was notified!).

#16 Updated by micahflee 2015-05-14 21:30:47

anarcat@koumbit.org wrote:
> This was 5 months ago. I don’t see anything in the upstream issue tracker, nor any details here about specific problems with onionshare, which is already available in Debian.
>
> Why isn’t it part of tails? It seems like useful software.
>
> I think responsible disclosure here implies sharing those issues with the public at some point, if upstream doesn’t do anything (assuming that upstream was notified!).

Hey, upstream developer here! The issues that jvoisin found were all very minor, and I’ve simply been swamped and haven’t fixed them upstream yet. I’m finally digging out some time to work on some onionshare bugfixes and release version 0.7, which will include all of the issues that jvoisin flagged. Hopefully I’ll have 0.7 released next week, at which point I think it would be good to move forward with getting it in Tails.

Here’s what I want to fix for 0.7:
https://github.com/micahflee/onionshare/milestones/0.7

These are the issues jvoisin found:
https://github.com/micahflee/onionshare/issues/168
https://github.com/micahflee/onionshare/issues/169
https://github.com/micahflee/onionshare/issues/170

In short, nothing serious at all. Although onionshare actually has a Tails-specific bug that started with a recent release of Tails:
https://github.com/micahflee/onionshare/issues/179

So I want to fix that for 0.7 as well.

#17 Updated by intrigeri 2015-05-15 11:24:04

> Why isn’t it part of tails?

Once the potential security issues are fixed, the integration / user story will need to be thought through. But that’s for another ticket.

#18 Updated by intrigeri 2015-05-15 11:24:57

> Although onionshare actually has a Tails-specific bug that started with a recent release of Tails: […]

That doesn’t look Tails-specific to me, but let’s discuss that on tails-dev@ rather than on an unrelated ticket :)

#19 Updated by micahflee 2015-05-19 00:51:11

I just released OnionShare 0.7, and all of the issues related to the audit have now been resolved. So I think this issue is actually done.

However, OnionShare in Tails is now blocked by a new issue:
https://github.com/micahflee/onionshare/issues/179
https://trac.torproject.org/projects/tor/ticket/16106

#20 Updated by intrigeri 2015-05-19 09:19:02

  • Assignee set to micahflee
  • QA Check set to Info Needed

> I just released OnionShare 0.7, and all of the issues related to the audit have now been resolved.

Great, thanks! The results of this initial audit are being tracked by Bug #8464, so I’ve asked jvoisin to confirm your claims there.

> So I think this issue is actually done.

Not exactly: after his initial audit, jvoisin wrote “I’ll be happy to audit it (more in depth) again once some worrying issues are fixed, and a clear design documented drafted/published”, so the next action on this ticket is blocked by:

  • Bug #8464 (as said above, and as expressed by the relationship between these two tickets already);
  • the lack of some design doc: is there any? I could not find it.

#21 Updated by micahflee 2015-05-19 19:02:30

There isn’t a design document yet, but I opened an issue to write one awhile ago: https://github.com/micahflee/onionshare/issues/167

I’ll get on that.

#22 Updated by micahflee 2015-05-27 18:07:41

I’ve written a security design document: https://github.com/micahflee/onionshare/blob/master/SECURITY.md

It will be included with the source code in the next release.

#23 Updated by BitingBird 2015-05-27 22:38:06

Wouah, bonus points for the design doc being human-readable, congrats!

#24 Updated by intrigeri 2015-05-28 08:53:31

  • Assignee changed from micahflee to jvoisin
  • QA Check changed from Info Needed to Dev Needed

> I’ve written a security design document: https://github.com/micahflee/onionshare/blob/master/SECURITY.md

Yay :) => reassigning to jvoisin for the next auditing steps he wanted to do.

#25 Updated by hybridwipe 2015-09-07 16:07:38

I ran the bandit static analyzer on it, thought I’d share it here.

#26 Updated by bertagaz 2015-09-09 01:10:22

Thanks a lot! It seems to raise interesting issues, in particular the predictable tmp directory usage. Did you send it upstream?

#27 Updated by hybridwipe 2015-09-09 01:14:40

bertagaz wrote:
> Thanks a lot! It seems to raise interesting issues, in particular the predictable tmp directory usage. Did you send it upstream?

I have not, please cc me if you do.

#28 Updated by bertagaz 2015-09-11 01:40:01

  • Assignee changed from jvoisin to micahflee

Assigning to micah then, for him to have a look at it. Please set the assignee back to jvoisin once done.

#29 Updated by intrigeri 2016-01-10 21:33:48

  • Assignee changed from micahflee to jvoisin
  • % Done changed from 0 to 30

intrigeri wrote:
> > I’ve written a security design document: https://github.com/micahflee/onionshare/blob/master/SECURITY.md
>
> Yay :) => reassigning to jvoisin for the next auditing steps he wanted to do.

@jvoisin: what’s the status on this one? Is there any remaining issue you have identified, and that is not fixed yet? If so, it would be awesome to list them here / point to GitHub tickets, except of course in cases when less rapid disclosure is preferred :)

bertagaz + hybridwipe: this ticket is about us auditing OnionShare, to make sure it satisfies our own security requirements. The idea is to the upstream author to specific problems, if we can find any. I don’t think it’s fair to send him the raw output of static code analyzers, and expect he’ll analyze the output himself: he never committed to do any such thing, while we did.

#30 Updated by jvoisin 2016-01-11 22:27:22

It seems that onionshare is under heavy refactoring to use ephemeral hidden services. There is little point in doing a security audit now.

#31 Updated by intrigeri 2016-02-19 00:45:57

  • Type of work changed from Audit to Security Audit

#32 Updated by jvoisin 2016-03-19 12:07:40

  • % Done changed from 30 to 100
  • QA Check deleted (Dev Needed)

I just did a review of the code, and found nothing alarming.
Great job michaflee!

#33 Updated by intrigeri 2016-03-21 09:59:56

  • Status changed from In Progress to Resolved
  • Assignee deleted (jvoisin)

Thanks!