comments are added to search index without checking access

Hm, a public security issue?

http://cyrve.com/criticals

Public security issues for D7 are fine until 7.0.

I added the permissions check in comment_node_update_index(). There's other issues though:

1. We only seem to render the first page of comments for the search index. Despite potential performance issues, we ought to render all of them, or add a new variable with a high limit and use that rather than the paging setting.

2. The links are a bit more thorny than that. They're added in comment_build_content() - while that has a concept of build mode, whether or not to add the links doesn't (although we can hack the 'in_preview' flag if there's no other way, wouldn't help with contrib links though).

That means either we either try to make comment links respect build mode, or we build our own versions of comment_view_multiple() just for search indexing, or we just eat it and add the in_preview flag with a @todo. None of these are great.

However, I'd say indexing the comment links and the number of comments we index, is a normal bug, whereas the information disclosure isn't. So marking CNR and we may need to split comment links into its own issue to tackle that properly. Also the reason we didn't have this issue in D6 was we didn't even render comments properly, just grabbed subject and body straight from the db.

so, comment_node_update_index takes a string, and not a render array? thats unfortunate.

$oermissions is a typo it seems.

Whoops.

IMO, comment links are extremely duplicative and useless info in 99.99% of cases. They should be omitted from the index. Meanwhile patch here is good to go once the bot is happy.

I just happened to be filing #721374: "Add new comment" is put into the search index along with each node when I came across this one. We can probably handle the comment links over there.

Renamed user_role_permission to user_role_permissions, let's see what the tests are saying now...

Ok, next try...

- Removed the todos
- The permission check was totally wrong, rid needs to be key when passing as an array to that function and the returned array is structured differently
- If I'm not mistaken, then we can just use user_access() in comment_node_search_result since we can base that decision on the currently logged in user...
- Added a simplified version of the search test without the permission and all the filter stuff to check if the permission checks actually work...

And with correct title now..

Looks good to me too, patch doesn't need search system expert IMO, it's just a permission check in comment module, comes with test, lovely.

I don't know. Adding comments to the search index only if anonymous users have permission to access comments, doesn't seem to make sense to me. What if I'm part of a private group and I want to search private comments?

It is my understanding that comments should always be indexed, but that they should never show up in the search results unless I have access to them. That is, we should do 'on output filtering' (on search) instead of 'on input filtering' (on index).

Yes, comment text is just added to the search index as part of the node text, there's no way to differentiate it. Nothing stops a contrib module implementing searchable comments if there was a demand for it.

Ah, that's right. So there are 3 options:

1) Index comments when anonymous users have access to them. This breaks search of private comments. This would also require the index to be (partially) rebuild when we change permissions. This is what this patch implements.

2) Index comments regardless of access. This could create a security issue as private comments might be exposed through search snippets.

3) Never index comments, and leave it up to a contributed module to do it right.

Yeah that's a good summary. Drupal 6 does #1 unless I'm very much mistaken.

Except there's also #1 + #3 = #4.

Hmm, and another possibility:

Index comments at the lowest level that users can both view comments and access search. So if anonymous users can search, but can't view comments, don't index. But if only authenticated users can search and view comments, index. That would handle an intranet situation, node access handles node access modules, might be enough for most sites.

That seems like not too much of a change, so setting back to CNR.

Don't have time to write that patch at the moment but seems doable and straightforward, and a lot better than blanketly denying access, so CNW.

Kinda related : there should probably be a separate 'search_index' view mode for comments, used when the comment is being rendered for indexing (currently done with comment_view_multiple() and the default 'full' view mode).
- Would let you adjust which comment fields get indexed,
- Would also let us make sure that field labels are not indexed (right now every node with a comment turns up when searching for '<label_of_a_comment_field>'), by explicitly forcing labels to 'hidden' when view mode is 'search_index' (currently hardcoded in http://api.drupal.org/api/function/field_default_view/7, but I'm preparing a more flexible, hook-based approach in #553298: Redesign the 'Manage Display' screen).

Not sure about a 'search_result' mode ? It seems currently we only display a ''@count comments'' string in search results, no actual comments ?

Can we simplify #22 to "Only index if there is no role that can search but not access comments"? Imho, roles don't have a level/order except maybe authenticated/anon.

Attaching a patch that does this and also extended the test to verify all auth/anon combinations.

I'm not fully convinced is this is enough, for example, what happens if the permissions change after the comments were indexed. Do we need to trigger a re-index? And should we note somewhere in the UI if comments are indexed or not....

Forgot the patch...

Patch looks good to me.

I don't think we fire any hooks when permissions change so that would require a submit handler being added triggering a reindex. Or we could get and set a variable in the code added here which already does this check and trigger it there. Reindex happens over time so if it's not instant that may not matter.

Mentioning it in the UI - seems reasonable but not sure where to put it.

Thinking about it, in Drupal 6 we never reindexed even if the comment module was disabled, let alone mentioning this in the UI. There's a manual button for triggering reindex. Some sites may make a number of changes then want to reindex - having it triggered for you with a lot of content could be extremely annoying. So I think this is plenty for now.

That's true, I was just playing "what if... " :)

Catch and I spoke about this issue. The short answer is, I support this patch, but have some minor improvements (follow up patch in a few minutes). My three cents are: (1) while the solution is a hack, but it's better to fix this security issue, (2) there are many places where the search index is not properly marked for update (for example #764798: Various actions make your search index out of synch - document this), and it's debatable about whether it's better to mark them or not, (3) and the real fix for search is to either keep multiple indexed content based on role, or field level indexing, both of which are too complicated for 7.x and we'll consider for 8.x.

These improvements change this so that we only have one query without an INNER JOIN, from two queries that both used INNER JOIN's.

This was RTBC, the only thing changed is how we check the role permissions. Just wondering why we have those fancy API functions in the first place when we don't use them, especially since this is only done once per index run anyway ;)

It looks like Dries has this one.

Fixed: null => NULL

Changed a query to DBTNG and optimized to return if there any roles without both permissions, same as replace foreach() with array_intersect()

My query is wrong should group by rid, also none of patches take into account inheritance of permissions

That query is not the same as the check above and there is no reason to use db_select() here.

What's wrong in your query:
- It is actually the opposite (tests should fail because of that)
- If the above would be correct, it would also deny to index if a role only has view comments but not search content. However, that is perfectly valid.

Not sure what you mean by inheritance. I don't think that permissions can be inherited. Single users can have multiple roles and in the end, a user can have search and access comments permissions yeah, but we simply can not test for that case. We don't want to, actually, because that could easily change.

Imho, #31 is still RTBC, but I'm leaving the decision to others since I basically wrote it (except for the query optimization).

About inheritance, my {role_permission} table

+-----+-----------------+---------+
| rid | permission      | module  |
+-----+-----------------+---------+
|   1 | access comments | comment |
|   1 | search content  | search  |
|   2 | access comments | comment |
|   2 | search content  | search  |
|   3 | search content  | search  |
+-----+-----------------+---------+

this happens after I disable access to search for RID (1 and 2) but enable for RID=3 (developer) and latter when search theming done I allow access to search for anonymous and authorized uses

In this case after finding role with 'search content' we *should* check if this rid>2 and check is rid=2 allowed to use search

+    // Prevent indexing of comments if there are any roles that can search but
+    // not view comments.

The right query should be

SELECT rid, permission FROM role_permission WHERE permission IN ('access comments', 'search content') GROUP BY rid HAVING COUNT(rid) = 1 AND permission='search content';

And after loop through roles and check for inheritance of permissions

A bit latter I provide a patch

.. and here is a patch!

1) Query used to select permissions, SUM() used to work for all databases (tested on mysql and postgres)
2) Changed test to check all states of permissions

I still don't see a reason to use db_select() as it is a static query :)

I'm using db_select() because it seems more readable and easy to change if permission names could change

@andypost, thanks! The previous patch (#31) was simpler query and fewer lines of code. Please explain why your patch is better, what it does that mine doesn't do, or why it's more efficient? Also, we need to make sure that it uses an index and can be cached, I'm concerned about the CASE WHEN clause. I'm still leaning towards the previous solution, but maybe I've missed something.

OK, in #37 I dump a part of my {role_permission} table

As you can see role 3 has rights to search but no right to view comments
But user with role 3 has rights to view comments because he owns/inherits authenticated role's permissions

Not sure about using indexes and cache for query but I think it should use primary key index because there's a filtering by permission and ordering by rid

"case .. when" could be done in code - this is a magic to be sure that role has "search" (1) permission by abstracting from role name.
It's a kind of bit-mask for permissions, where the "count" tell us about how many permissions the role has.

Truth table

count | search_permission => 'access comments' | 'search content'
1     | 0                     true             |  false
1     | 1                     false            |  true
2     | 1 (always)            true             |  true

Also reworked test should explain better this cases with permission inheritance

@andypost, Ah, Ok, good catch, so what my last patch was missing, is that all roles authenticated user roles inherit from the authenticated role. That's really a simple fix to the patch in #31, see attached. I think that this is simpler in terms of SQL and code, than #38.

Latest patch is based on #31 with this extra check for auth roles inheriting from auth role, with the NULL change, with the latest tests from #38, but with a few tweaks to those tests so that the exact check that passes/fails is included in the output (basically what was the comment is now a string as part of the result).

I think we are missing a test where 'search content' is inherited, but might of missed it?

Yes it looks like we're missing that particular combination. I don't have the energy to re-roll, nor the heart to mark this needs work, should be a relatively easy fix though.

unfortunately in #21 the comment about Drupal 6 doesn't hold - in fact this bug exists in Drupal 6 and 5 as well (as well as likely a number of contributed search modules).

Given the fact that this issue has been public for so long (and also that no one has ever reported to us that they set up a Drupal 5 or 6 site in such a way that they noticed the access bypass), our conclusion within the Drupal Security Team was that we should just finish this issue asap and do any needed backports or contrib fixes in public as hardening issues.

I'll do it. :) Sounds like catch thinks this might be somewhat easy, marking it novice. #44 says it needs a small fix and a re-roll.

#43: 680992.patch queued for re-testing.

Hello.
I'm a novice in the Drupal, but I wish to ask you guys why wouldn't you solve permissions problems by doing comment type like another content types?
I write a little topic on forum about that - http://drupal.org/node/801950
And I know there is a module Node comments that is doing something like.
So, is it would not solve the problems with permissions?

We don't have per-content type view permissions in core, so that would actually make the permissions less granular than they are now out of the box, additionally the idea with adding comments to the node search index is you often want to find a combination of words which may not be in any one node - searching for issues, forum threads etc. are obvious examples.

Going to mark this for another retest because that failure looks like a test bot glitch.

#43: 680992.patch queued for re-testing.

hm, I've heard the CCK will be in Drupal 7 core, isn't it? It won't add a per-content type view permission?
And about per-page search ("combination of words which may not be in any one node") why are you so sure that the ability to mark some content type as comment will not enable this feature? Why is it not possible to estimate permissions by content type and realize search options by comment-to-node relation?

Sorry about my questions but I'm really interested in this possibility and wanna understand.

Sorry again.
You're right that we don't have any view permission. Isn't it a little bug too?
I mean we often need to have a possibility to enable user view only his own nodes most often it is for comments. And in this case we need to set author of main node permission that way to enable him view all comments to his node...
And also all this is needed when user use the search.

Any kind of per content-type view permissions in core is a feature request for Drupal 8 (and there are issues for that already existing, can look in http://drupal.org/project/issues/search), it can be achieved with a contrib module very easily - the patch here is completely right, just needs updating for the extra test. Also the field and entity APIs in Drupal 7 make the arguments for having comments as nodes much less valid than they used to be.

@catch
thank you for your explanation.

Just a re-roll for now, still needs test improvement as mentioned in #43.

• Re-rolled to keep up with HEAD
• Added the missing test (inheriting the "search content" permission from the authenticated user role.)
• Changed
  

    $index_comments = &drupal_static(__FUNCTION__, NULL);
  

  to

    $index_comments = &drupal_static(__FUNCTION__);
  

  as NULL is the default value for drupal_static

This patch does not yet address the re-index on permission change concern in #25…

...and once more with a tangible patch...

Looks good, tests pass, RTBC.

Same patch as #60 but with one additional line-break.

Still, needs review then :) Can be put back to RTBC when green.

Testbot approves, back to RTBC.

Reading Dries's concerns in #17 and #20, it looks like the current solution is a nice balance of sane default behaviour, while also preserving the ability for a private comment contrib module to implement its own hook_node_update_index() to deal with the situation differently. Tests are very thorough and lovely. The indirection of those wrapper functions like checkCommentAccess() and setRolePermissions() is a bit odd, but I can see why that's required given all the various edge cases that are being tested.

Committed to HEAD! Marking to be ported for 6.x (I think?)

minor style nit wrt the comitted patch:

 if ($index_comments === NULL)

don't we usually use !isset() or is_null()?

I think we probably should backport this, though it shoudl probably include an update function if we need to force reindexing?

subscibing and because it is privacy / security related I would like to bump this to be ported

Patch (To be ported) for over a year now with a passing test? Bumping. What is blocking this from being ported? Sure would be nice to close this.

Still a bug and a security exposure as it reveals comments on a node protected with content access. Worse yet, if you click on the comment in Recent comments block you get to see the whole protected node. Admittedly the comments were made before the content type was protected. Perhaps clearing the index fixes this?