SA-2008-63 saw a rash of contrib modules that were improperly updated to the new menu system. The fact that we use booleans for indicating access always allowed or never allow turns out to make it easier than it should to make an access bypass by doing a function call to user_access() (for example).

To harden this piece of the core APi going forward, chx proposed and I implemented a minor modification to the menu system to used defined strings rather than booleans.

CommentFileSizeAuthor
#7 access-callback-harden-D7-319360-7.patch6.49 KBpwolanin
#5 access-callback-harden-D7-319360-5a.patch5.79 KBpwolanin
#1 access-callback-harden-D7-319360-1.patch1.91 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Title: Strengthen 'access callback' APi in menu system » Harden checking of 'access callback' in menu system API
Status: Active » Needs review
StatusFileSize
new access-callback-harden-D7-319360-1.patch1.91 KB

pretty simple patch.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Could I have a bit more information? How are contrib modules getting this wrong?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I'm not sure that we should make things a little bit harder when contrib modules can't do access control right. But that's my opinion...

catch’s picture

catch
he/him
Primary language English
commented

People are putting

'access callback' => user_access('use PHP');

which evaluates as TRUE.

afaik, it doesn't make any more work in contrib, just stops poorly upgraded D5 modules from having access bypasses when they completely ignore how the new menu system works.

pwolanin’s picture

pwolanin commented
StatusFileSize
new access-callback-harden-D7-319360-5a.patch5.79 KB

here's a patch that actually converts the few uses in core.

All tests pass with this patch: 6884 passes, 0 fails, 0 exceptions

@Dave Reid - this patch should make it harder for contrib to get it wrong, but not at all harder to get it right.

pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

looks like this needs work

pwolanin’s picture

pwolanin commented
StatusFileSize
new access-callback-harden-D7-319360-7.patch6.49 KB

looks like the problem was just soe additional items in a test module that the prior patch misses

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
moshe weitzman’s picture

moshe weitzman commented

IMO, this is a result of people doing a rushed d5 to d6 upgrade. perhaps coder or deadwood contributed to the problem - i don't know. anyway, the problem is gone with d6 => d7 so i really think this is not very useful. IMO, this is babysitting. I don't feel strongly though.

dries’s picture

dries commented

I'm not particularly fond of this patch, to be honest.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Needs review

submitting for re-testing after snafu this afternoon.

Status: Needs review » Needs work

The last submitted patch failed testing.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Closed (won't fix)

Looking at the comments and the lack of activity, I guess this is a "won't fix". Correct me if I'm wrong.