Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
As noted by David Rothstein we should work on this patch in a public case. Latest patch attached.
Critical for RTM release.
Comment | File | Size | Author |
---|---|---|---|
#13 | 1441574-Port-D6-access-bypass-bugfixes-to-D7p4.patch | 34.47 KB | hass |
#11 | 1441574-Port-D6-access-bypass-bugfixes-to-D7p2.patch | 34.24 KB | hass |
#9 | 1441574-Port-D6-access-bypass-bugfixes-to-D7.patch | 34.02 KB | hass |
#6 | check_node_access_for_reports-1441574-6.patch | 1.97 KB | tjmoyer |
D7_linkchecker-security-4-git.patch | 31.29 KB | hass | |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedFor reference, this is the followup for http://drupal.org/node/1441252.
I'm pretty busy the next few days, but will try to find some time to work on this soon.
Comment #2
hass CreditAttribution: hass commentedFirst bug report for the security changes #1442080: "permission restrictions" in reports -> run re-analyze only task once after upgrade
Additional to this I believe this changes will cause major troubles. If a link get's broken and it's used in let's say 1.000 or 10.000 nodes, all this nodes are loaded. Every installation will go out of memory... This need to be changed or we need to rollback the committed patch.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedThe code we added doesn't load all the nodes; it's supposed to stop after the first one in which the user has access to see the link. So the only time it should load lots of those nodes is if the user viewing the broken link report does not have access to the vast majority of them. I thought we decided that's a very unlikely scenario... Did you come across another way to trigger that?
I'll follow the internationalization issue now - nodes in different languages is definitely not something I tested :( So it sounds like there may be an issue that needs fixing there.
Comment #4
hass CreditAttribution: hass commentedI have no good idea how to solve the 10000 node load issue, but it will happen and this is extremly bad... :-(((. The i18n issue can be a false report... Maybe he really had no permission...
Comment #5
hass CreditAttribution: hass commentedWe need to take care of i18n, #1488572: i18n: "Permission restrictions deny" all broken links in other languages, #1489104: I18n db rewriting breaks linkchecker
Comment #6
tjmoyer CreditAttribution: tjmoyer commentedThere doesn't seem to have been any progress or movement here in many months. I have been working on a module to tie in linkchecker with OG to provide a linkchecker report to groups managers, but found that linkchecker's queries for the report page don't respect node access.
I have created a patch that respects node access in the _linkchecker_report_page() function. This ensures that anyone who views a report only sees links to edit nodes they have permission to edit.
Comment #7
hass CreditAttribution: hass commentedI worked yesterday on the patch and fixed some bugs, but was not yet able to complete the node function. I hope to be able to post the patch soon.
Comment #8
hass CreditAttribution: hass commentedComment #9
hass CreditAttribution: hass commentedI think I have finshed this task now.
@David_Rothstein: I know you are bussy with core, but is there a chance for a security review prior to commit?
Comment #11
hass CreditAttribution: hass commentedSuxxx Eclipse Egit changes. New try with older Eclipse 3.7.2
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedThanks for reviving this issue. I reviewed the code (but did not test it out yet). Looked mostly good, but:
db_rewrite_sql() doesn't exist in Drupal 7; the replacement for node access is $query->addTag('node_access'). So the two queries below this comment need to add that in order to actually allow node access to be enforced here.
Similar issue here as above, although I'm not 100% sure how things actually work for comment access in Drupal 7... Perhaps it just needs a $query->addTag('node_access') line too? (Since comments have a node.nid column in the database, and this should allow node access modules to exclude comments that are attached to nodes the user doesn't have access to.)
The third parameter to field_access() should be 'node', not NULL.
Does that last line actually work? Regardless, I think
->isNull(r.rid')
is supposed to be the best way to do that.Comment #13
hass CreditAttribution: hass commentedI'm not familiar with
addTag('node_access')
at all and wasn't aware of this. This are not really security bugs to me, I see this more like a missing node_access feature. I have also addedaddMetaData('base_table', 'comment')
per http://drupal.org/node/1204572, but I do not understand how this works internally. This is a major DX failure to name'node_access'
, but using taxonomy tables and also write for every node listing query above, but may be a bug in this page!? Than http://drupal.org/node/310077 is also more confusing and also has aentity_field_access
(anything required for this?). Based on this I guess http://drupal.org/node/1204572 should use'term_access'
and not'node_access'
, but I have no clue and also not how to test all this variants. Who can we ask?Good catch, I wasn't aware of
->isNull()
, but this makes sense as this is really not DB independed otherwise.Aside, not using
addTag('translatable')
in the node querys sounds like this will solve the i18n issues in D7 by itself and I do not need to do any extra exclusions as required in D6 #1488572: i18n: "Permission restrictions deny" all broken links in other languages.Comment #14
hass CreditAttribution: hass commentedOpened #1857592: "Converting node access queries to 7.3" documentation is confusing
Comment #15
hass CreditAttribution: hass commentedAside running function
_linkchecker_link_access($link)
on a block edit (admin/structure/block/manage/block/1/configure) sound pretty useless to me.First two selects for nothing (possible followup):
Comment #16
hass CreditAttribution: hass commentedKnown follow up bug in this patch #1857620: hook_form_alter() fails on BASE_FORM_ID 'comment_form'.
Comment #17
hass CreditAttribution: hass commentedCommitted #13 for now as this issue is blocking me from working on all other issues.
http://drupalcode.org/project/linkchecker.git/commit/cb234f4
Comment #18
hass CreditAttribution: hass commentedRelated follow up #1804842: Lots of warnings when comment/block modules are not installed
Comment #19
hass CreditAttribution: hass commented@David: To fix #1489132: "Permission restrictions deny" message on many broken links when https securepages we need to ignore the protocol in
_linkchecker_extract_node_links()
atif (strpos($item, $original_link) !== FALSE) {
. Do you see a problem with removing the protocol (https?://
or onlyhttps?
)?