_node_revision_access() is a menu callback that checks whether the current user can do a particular operation to a node. Like a good citizen, it caches its results in a static variable.
However, it only caches the vid of the node it checked access for. It omits the operation. This means that if the callback is used to check access for two different operations in the same page load, only permissions for the first access check are reflected. For example, if the current user has the 'view revisions' permission but not 'update revisions':
_node_revision_access($node, 'view'); // returns TRUE
_node_revision_access($node, 'update'); // also returns TRUE, should return FALSE!
This function is an 'internal' function by its naming pattern, and folks can work around this bug by clearing the '_node_revision_access' static variable (drupal_static('_node_revision_access', array(), TRUE)
), but this should still be fixed. Patch attached.
Comment | File | Size | Author |
---|---|---|---|
#33 | 1064954-d7.patch | 7.21 KB | xjm |
#28 | 1064954-node-revision-access-fix-static.patch | 6.87 KB | Dave Reid |
#27 | 1064954-node-revision-access-fix-static.patch | 6.87 KB | Dave Reid |
#25 | 1064954-node-revision-access-fix-static.patch | 6.13 KB | Dave Reid |
#24 | 1064954-node-revision-access-fix-static.patch | 6.12 KB | Dave Reid |
Comments
Comment #1
BerdirInteresting, that patch should not have passed. Sounds like a bug in the testbot to me.
When creating patches, make sure that the new file name (file name on second line of the patch) matches the actual filename of the file. You should be able to fix it manually directly in the patch and re-upload.
I guess a test would be useful here as well and this will need to be backported to D6.
Oh and for anyone reading and wondering, I've check with the security team that this can not be exploited.
Comment #2
becw CreditAttribution: becw commentedAh, that was a silly error on my part!
And it's true, this can not be exploited unless someone reuses it in a non-core module--which is not recommended, since the function name begins with an underscore (this convention indicates something is meant as an "internal" function). However, if a module is interacting with revisions, it may be tempting to re-use this function in order to re-use the core logic for determining revision access.
Corrected patch attached, no tests though.
Comment #3
BerdirRemember to set issues to needs review so that patches are tested.
Comment #4
gregglesGiven that there are no known vulnerabilities in core or contrib associated with this bug it can be fixed publicly, but in general this kind of thing should have followed the process to report a security issue first and then could be made public after that.
Comment #5
gregglesAnd the priority should probably be higher.
Comment #6
becw CreditAttribution: becw commentedThis is an issue for D8 as well. The patch from #2 applies to D8 with some fuzz. Here are D7 and D8 versions that apply cleanly.
Comment #7
catchComment #8
becw CreditAttribution: becw commented#6: 1064954-6-d8-node_revision_access.patch queued for re-testing.
Comment #9
catchMoving this to critical since it's security (even if probably very minor), and CNW for tests.
Comment #10
becw CreditAttribution: becw commentedI'm sorry, what does "CNW" mean?
Also, what work does this patch need? I posted versions for both Drupal 7 and Drupal 8 in comment #6, and they both passed testing against their respective branches.
Comment #11
catchCNW/code needs work.
Needs tests because the existing tests we have aren't catching the bug, so we should add additional test coverage that would fail without the patch being applied.
Comment #12
xjmI'll work on a test for this.
Comment #13
xjmSo, here's a test. Couple notes:
_node_revision_access()
because otherwise it uses the uid of the parent site session.node_test.module
.view revisions
permission, we'd need to add node access into the mix, because otherwise the global all grant obscures access to the revision. (That, and/or theview revisions
permission doesn't actually do anything and it shouldn't exist.)setUp()
is from other node revision tests.Comment #15
xjmThe bot was sick: #1459524: Testbot #659 fails with FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to drop checkout database..
Please review. :)
Comment #16
xjmWrong diff apparently; there are a couple of inline comments I'd added to this test method that are missing here. I'll fix that.
Comment #17
xjmAdded those comments back. I also adjusted the test method to use
assertNoUniqueText()
(as the text should appear twice). That was naggling overnight.Comment #18
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThe code style looks great, except for these two:
permssions -> permissions.
Superfluous space.
I've tried to follow the logic and it looks ready to go for me, but I'm not that familiar with the revision system. The test fails without the fix and passes with the fix, so what's left to ask for? :)
Comment #19
xjm@Tor Arne Thune, I don't see the superfluous space?
Comment #20
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedTwo spaces in front of
$this->node_revisions[1]->vid);
Comment #21
xjmSo there are. :) OK, cleaning that up too.
Comment #22
xjmAforementioned style fixes; no other changes.
Comment #23
xjmOops, #22 is incomplete.
Comment #24
Dave ReidHere's my proposal to fix this issue by adding an optional $account parameter for _node_revision_access() which appropriately gets passed along to all the user_access() and node_access() calls. This change is safe to backport to D7 as well since it is an API addition rather than a change. This patch also adds tests for a user with the 'administer nodes' permission, as well as a user who has no revision-related permissions at all.
Comment #25
Dave ReidWithout undefined $map in the test assertion message this time.
Comment #26
xjm$account
parameter drove me bonkers when I was working on this, I was really reluctant to add it here because it felt like scope creep. However, the patch is so much nicer that I think I'm changing my mind.generatePermutations()
is an awesome method I didn't know about.setUp()
for the other test case in this manner as well.Typo: "obejct"
Looks like these comments are ever-so-slightly over 80 chars.
Comment #27
Dave ReidFixes issues addressed in #26 and also adds test coverage for invalid $node and $op parameters, as well as testing that $account defaults to the global user object.
Comment #28
Dave ReidBLARGH GRAMMAR
Comment #29
xjmI approve of this patch.
Comment #30
webchickI was a little bit concerned about:
However, ultimately this is just an internal refactor, that moves the access check from 2D to 3D. Seems sensible to me, and comes with good test coverage.
Committed and pushed to 8.x. However, looks like it needs a re-roll for 7.x.
Comment #31
xjmOkay, so wow, this function doesn't even have a bedamned docblock in D7? How did that happen? I'll work on the reroll.
Edit: Ah, #1313980: Clean up API docs for node module never got backported. Going to backport the full docblock anyway since this is a critical (duh). That node docs backport is going to be damn hard to reroll already anyway.
Comment #32
webchickBedamned is officially my new favourite word.
Thanks, xjm!!
Comment #33
xjmNeeds review since I did some cut-and-paste action by hand. Also trying out for the first time using
-d7
to mean simply "this is a patch for d7" rather than "testbot should categorically ignore this patch under all circumstances." Yay!Comment #34
amateescu CreditAttribution: amateescu commentedThe D7 reroll looks good to me. Down with criticals!
Comment #35
webchickAwesome, thanks a lot for the fast turnaround on this, folks!
Committed and pushed to 7.x.
Comment #37
anrikun CreditAttribution: anrikun commentedWhat about D6?