Modify

Opened 5 years ago

Closed 4 days ago

Last modified 4 days ago

#5784 closed defect (cantfix)

Loss of the plugin causes sensitive information disclosure

Reported by: antti-juhani@… Owned by: dkgdkg
Priority: normal Component: SensitiveTicketsPlugin
Severity: major Keywords: security precaution
Cc: antti-juhani@…, rjollos, mmitar@…, lkraav Trac Release: 0.11

Description

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.

Attachments (0)

Change History (15)

comment:1 Changed 5 years ago by rjollos

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.

comment:2 Changed 5 years ago by 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:)

comment:3 follow-up: Changed 5 years ago by 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.

comment:4 Changed 4 years ago by obs

  • Owner changed from sbenthall to obs

comment:5 in reply to: ↑ 3 Changed 3 years ago by hasienda

  • Cc rjollos added
  • Keywords security precaution added

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.

comment:6 follow-up: Changed 3 years ago by mitar

  • Cc mmitar@… added

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?)

comment:7 follow-up: Changed 3 years ago by 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.

comment:8 in reply to: ↑ 6 Changed 3 years ago by hasienda

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.

comment:9 in reply to: ↑ 7 Changed 3 years ago by hasienda

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?

comment:10 Changed 3 years ago by 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.

comment:11 Changed 3 years ago by lkraav

  • Cc lkraav added

comment:12 Changed 2 years ago by hasienda

  • Owner changed from obs to dkgdkg
  • Severity changed from normal to major

Thanks mitar for linking to the related issue for Trac.

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

comment:13 Changed 2 years ago by dkgdkg

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

comment:14 Changed 7 months ago by rjollos

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.

comment:15 Changed 4 days ago by rjollos

  • Resolution set to cantfix
  • Status changed from new to closed

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.

Last edited 4 days ago by rjollos (previous) (diff)

Add Comment

Modify Ticket

Action
as closed .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from dkgdkg. Next status will be 'closed'.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.