Page MenuHomePhabricator

Ensure Phab upstream deprecations (_WILLEDITTASK and _DIDEDITTASK) don't compromise our security
Closed, ResolvedPublic

Description

On 2015-11-26, epriestley filed Phab Upstream T9860: "Upgrading: TYPE_MANIPHEST_WILLEDITTASK and TYPE_MANIPHEST_DIDEDITTASK are deprecated and will be removed". This concerned us, because we rely on those methods to keep Security issues secure. Based on epriestley's advice, we believe that we have a plan, but this task is about the "verify" half of "trust, but verify"

Event Timeline

mmodell raised the priority of this task from to Needs Triage.
mmodell updated the task description. (Show Details)
mmodell triaged this task as Medium priority.Dec 1 2015, 8:20 PM
mmodell set Security to None.

@mmodell, can you specify what you're looking for on this ticket-- Consensus to change our process? For us to all petition upstream to change their mind? Brainstorm other options?

My reading of the comments on Upstream T9860 is that Evan was able to explain his plan, possibly tweaked their plans a little bit to accomodate us, and explained some of the advanced (new?) Phab features that we might not have been utilizing fully. There was a bit of back and forth between Evan and Mukunda as Evan learned more about what we were doing, and Mukunda learned more about Phab-fu.

It looks like it resulted in a productive conversation about how to move forward. @csteipp, assuming I'm characterizing the upstream conversation, it might still be worth having a security discussion with Mukunda about his near-term plans for merging/not-merging/whatever. @mmodell, do you have near-term plans for another upstream merge, or are you going to wait until the dust settles upstream?

@RobLa-WMF: No immediate plans to merge. I will most likely need to do a bit of testing before the next upgrade. I don't think it's urgent, given that we don't have any major problems with the current version.

At first: I thinked that we can disable the herald rule and the seucity extension (including the field) when upgrading, and use this system: https://secure.phabricator.com/book/phabricator/article/forms/#use-case-security-issues

It looks like the effect is the same, the only difference is, that if the user wants to change the policy after creating a task, then he has a problem. So we have still something a lot to do :-/

Offers:

  • Allow anyone to change the visibilty options
    • The user can adjust the options if he failed choosing the right form
    • Users can abuse this function and hide important tasks (members with console access have to unlock them, but it's not easy to find them)
  • Rewrite the herald rule
    • Allows to adjust the visibilty after creating
    • Don't needs event listener configured anymore (Event.listeners is just needed for adjusting the options when creating a task; in this case we need only herald if we want to adjust them after creation)
    • Work
  • Don't use the new options (edit forms etc.) and rewrite the security extension
    • Current system does not change, users don't have problems with new forms and choosing the right options at creation
    • Not possible to use edit forms if we want to realise specific workflows (I don't know if we want), but we miss a good change to make phab more flexible and after this easier for bug reporters etc.
    • we still have to rewrite the extension

That are only the points, that I'm able to find at the moment, maybe you have more...

RobLa-WMF renamed this task from Panic? Upstream just deprecated the events that we use to implement the Phabricator security extension to Ensure Phab upstream deprecations (_WILLEDITTASK and _DIDEDITTASK) don't compromise our security.Dec 13 2015, 3:33 AM
RobLa-WMF updated the task description. (Show Details)

At first: I thinked that we can disable the herald rule and the seucity extension (including the field) when upgrading, and use this system: https://secure.phabricator.com/book/phabricator/article/forms/#use-case-security-issues

It looks like the effect is the same, the only difference is, that if the user wants to change the policy after creating a task, then he has a problem.

We don't allow users to do this in the current system. You need to directly edit the visibility policy (privileged action) to actually make something public, even if you set the security dropdown to none.

@Krenair Sorry, I mean not in this direction. I mean:

  1. You create a task
  2. You decide "looks like a security bug"
  3. You change the dropdown from none to ssb
  4. Herald changes policy

In this case the new system will not work, if the user don't have the right to change the permissions.

Maybe an alternative way (not the best way):

  • Create a space for securtiy bugs, and allow users to change space, when editing the form (I don't know if it's fullsy possible):
    • Don't need the security extension, no problem with abuse (like setting the visibilty option to "no one")
    • With this space it's not possible to allow CCs to view the task (and maybe the task autor can't see his task too, I don't know)

Oh, I see what you mean. But yeah, using the spaces system is a non-starter.

So it would be better to choose the ways at my first comment (or others, but I don't know others at the moment). I'm prefering the second options, because this let us realise more specific workflows, if we want, and there is no security or abuse risk.

mmodell claimed this task.