Loss of the plugin causes sensitive information disclosure

[antti-juhani@…]

If the plugin is disabled for whatever reason, any sensitive tickets become visible to everyone who has TICKET_VIEW privileges (which is often everybody). The security implications of this are chilling, I'd say, especially since the plugin is expressedly intended to hide such tickets and a seemingly innocious action like upgrading the python interpreter might (according to ​http://trac.edgewall.org/wiki/TracPlugins) disable the plugin.

[Ryan J Ollos]

The core issue may be that the plugin can only revoke permissions after TICKET_VIEW is granted, so the behavior is inherit in its design. The same issue seems to exist with the PrivateTicketsPlugin.

[antti-juhani@…]

I agree that it's inherent – it's a fail-open design instead of a fail-closed. But would the following work?

• Provide two permissions, SENSITIVE_VIEW and INSENSITIVE_VIEW
• Require SENSITIVE_VIEW for a sensitive ticket and INSENSITIVE_VIEW for any other ticket
• Document that TICKET_VIEW should not be granted to anyone, and that INSENSITIVE_VIEW should be used where one would normally use TICKET_VIEW.

(Obviously, INSENSITIVE_VIEW is an atrocious name, and should be renamed:)

[Ryan J Ollos]

I suspect the design was done as is because it is easier to insert a hook that checks for a newly defined permission before granting access. We'd have to dig into the details of the code to figure out for sure.

[Steffen Hoffmann]

Replying to rjollos:

I suspect the design was done as is because it is easier to insert a hook that checks for a newly defined permission before granting access. We'd have to dig into the details of the code to figure out for sure.

Sure, this has been a "most effect by least effort" development so far.

Question is, if it's worth the hassle to redesign. For professional use I think it is. Information disclosure is unacceptable in some businesses regardless of the reason.

I've been thinking about this issue for months by now. There'll be a trade-off to choose between minimum interference with current and future Trac ticket handling and maximum security against unwanted information leaks.

1. For backwards compatibility and use cases, where a rather low level of confidence is acceptable, the current logic should be retained.
2. For maximum security we'd want to have all potentially sensitive information
   a) removed from standard db tables (ticket, ticket_change, ticket_custom and even attachment and the common attachment store in the filesystem) or
   b) at least encrypted state-of-the-art (transparent en-/decryption comes to mind here)

While just moving out content seems like an easy start, I think it would be ugly to duplicate the aforementioned db tables and storage structure at the filesystem and bothersome, if feasible at all, to merge tables dynamically on-demand and on-permission for all kind of access (i.e. by TicketQuery, TracReport, Milestones views, etc.).

Currently I tend towards wrapping the core ticket handling operations (create, read, change, append attachment and delete possibly too) in such a way, that we have a customizable disclaimer in summary, possibly usable as a trigger value too, and all sensitive date inside of an encrypted container string stored in the ticket description. It could be adjustable, how many/which ticket fields should be obfuscated, i.e. allowing check and evaluation of ticket status and milestone assignment could still be acceptable. Either way we'd fix the issue, while for industry grade information protection I prefer content encryption over just moving data. It could establish high confidence even at the db level.

I'll try to make this happen within some time, most probably building on top of an existing GnuPG wrapper, as I've done in development for AnnouncerPlugin/MessageEncryption.

[Mitar]

I think you are complicating here too much. Encryption? Isn't it easier to make whole partition where database and attachments are stored encrypted? Why would we store things in the database unencrypted? Do not forget, if Trac will have access to decrypted content then any bug in it (or unauthorized access through some privileged user) will give this content. I think this is the wrong approach.

I believe we should solve the problem of disabled plugin. We should trust other components to do its job (from operating system to Trac and so on). This idea of replacing TICKET_VIEW with something else is interesting because it might maybe solve the #4619 problem.

Wouldn't it be easier to simply set required to True for this plugin and hopefully make Trac not start if it is unable to load it. (Currently it seems this flag does nothing? Why it is defined in Trac components then?)

[Mitar]

Probably this is the general Trac problem of its permission policy components. Maybe it would be easier to add another configuration option next to the permission_policies, like require_permission_policies_components which would make Trac not even start if it cannot load all components defined in permission_policies. I would even set this by default to True (I can hardly imagine an example where it would be useful that Trac starts anyway, maybe this would even not be a configuration option but just simply a start-up check).

So I think this is Trac security problem. Not something plugins should take care. But their environment.

[Steffen Hoffmann]

Replying to mitar:

I think you are complicating here too much. Encryption? Isn't it easier to make whole partition where database and attachments are stored encrypted? Why would we store things in the database unencrypted?

I don't see how this could help with selective hiding some ticket content from the Trac db.? Still...

Do not forget, if Trac will have access to decrypted content then any bug in it (or unauthorized access through some privileged user) will give this content. I think this is the wrong approach.

This is true for most plugins. They're all trusted ultimately AFAIK, just as Trac native code, right?

I believe we should solve the problem of disabled plugin. We should trust other components to do its job (from operating system to Trac and so on). This idea of replacing TICKET_VIEW with something else is interesting because it might maybe solve the #4619 problem.

+1

Wouldn't it be easier to simply set required to True for this plugin and hopefully make Trac not start if it is unable to load it. (Currently it seems this flag does nothing? Why it is defined in Trac components then?)

Didn't notice this before. Now I'd like to know this myself.

[Steffen Hoffmann]

Replying to mitar:

So I think this is Trac security problem. Not something plugins should take care. But their environment.

I was mentioned before, that an opt-in (deny by default) would be better design for permissions than current opt-out architecture. I tend to agree to this, so I do agree, that this should be better resolved in Trac itself. But do you know of an existing ticket for Trac, demanding this?

[Mitar]

Yes, deny by default would be an option, but it would require many plugins to change/adapt. I think this is why it is easier to request that all components in permission_policies should be loaded.

It is ​now.

[Steffen Hoffmann]

Thanks mitar for linking to the related issue for Trac.

Assigning to current maintainer now for a follow-up on this rather important discussion.

[Daniel Kahn Gillmor]

I've followed up on ​the core ticket. I agree that without this, the SensitiveTicketsPlugin is dangerously brittle.

[Ryan J Ollos]

​trac:#10285 has been resolved on the ​browser:/branches/1.0-stable branch and will be part of the forthcoming Trac 1.0.2 release.

[Ryan J Ollos]

I've updated the documentation. I don't think anything can be done in the plugin, so I'm going to go ahead and close the ticket.