Make fields check more useful with risky content (allowlist of content?)

I was thinking about this same thing recently, and thought a whitelist of MD5 hashes of the field content would be best because then it's approving not just a particular entity, but that we know that the actual contents are known to be good. If someone updates the field later with changed script/PHP then we have to re-approve it, but I feel like that's the right thing to do because then you know the new content is trusted too.

Ah, sorry, I see that you mentioned using an MD5 hash in the issue summary -- I some how skipped over that and just saw the entity id and field. Sorry for the extra noise!

I haven't tested this and only did a very, very quick look at the code so far but one comment right away:

+++ b/security_review.inc
@@ -518,23 +518,28 @@ function security_review_check_field($last_check = NULL) {
+          if (!in_array(hash('sha256', implode((array) $result)), $known_risky_fields)) {
+            $check_result = FALSE;
+            $check_result_value[$result->entity_type][$result->entity_id] = array(
+              'type' => $vuln_type,
+              'field' => $info['name'],
+              'hash' => hash('sha256', implode((array) $result)),

This will hash twice in the case that a field isn't in the list of known risky fields, which will probably be most fields.

Of course, doing sha256 isn't super performance intensive, but it would make sense to me to stash the hash value in a variable to avoid doing it twice for every value in the majority case.

I just tested this and it worked as expected! A field on the "Settings" page would make configuring this field way easier (both to do and to explain) but I'm not sure we should make it that easy...

Another super minor bit of patch review:

1. +++ b/README.txt
   @@ -95,6 +95,14 @@ locations, then:
   +that you want to be skipped in future runs by creating a sha2 hash of the
   

   I'm not super fluent in the different hashing algorithms but it appears that sha-2 is a superset that includes sha-256:

   https://security.stackexchange.com/questions/87154/what-is-the-relations...

   The docs here should probably say sha-256

2. +++ b/security_review.inc
   @@ -518,23 +518,29 @@ function security_review_check_field($last_check = NULL) {
   +    $sql = "SELECT DISTINCT entity_id, entity_type, " . db_escape_table($info['column']) . " AS field_text FROM {" . $table . "} WHERE " . $info['column'] . " LIKE :text";
   

   Using db_escape_table() to escape a column is very D6 of you. :-) I'm not totally sure we even need to escape here, as the column is coming from the Field API, and not user input, so probably not? But if we do choose to escape it, it should be db_escape_field() and we should probably also escape $table, which also comes from the Field API and so is equally trustworthy.

Anyway, besides those super minor things, this looks great!

Looks great to me!

I apologize for the somewhat ignorant question, but I have content I want to whitelist, and security_review has generated the hash I want to put in the whitelist. My question is, where should I put the whitelist? Where is it supposed to live? The README doesn't specify. Thanks in advance.

Yes, #16 did it for me. Thank you.

Here is a first D8 version of the patch.
For now without any help or README stuff.

I added the config setting known_risky_fields to security_review.check.security_review-field (i.e. security_review.check.security_review-field:known_risky_fields).
It contains sha256 hashes of: entity-type, entity-id, field-name and content.

Because I think a yaml config should be as human readable as possible known_risky_fields is an array (not a comma-separated string like in the D7 version).

For now you can set the values via drush:
php -r "print yaml_emit(['hash1', 'hash2', 'hash2']);" | drush cset security_review.check.security_review-field known_risky_fields --input-format=yaml -
The trailing " -" is needed (to read from stdin).

I think we should allow setting known_risky_fields via UI ("Check-specific settings").

New version of #18:

• Added CheckSettings/FieldSettings to set known_risky_fields via UI ("Check-specific settings").
• Accordingly changed yml config from security_review.check.security_review-field:known_risky_fields to security_review.check.security_review-field:settings.known_risky_fields

You can also set known_risky_fields via drush:
php -r "print yaml_emit(['hash1', 'hash2', 'hash2']);" | drush cset security_review.check.security_review-field settings.known_risky_fields --input-format=yaml -

Testing / using #19 I wondered if we should allow a comment to each hash in known_risky_fields to remember the reason why a certain content is considered safe.

That would change the setting to an associative array ('hash1' => 'field body in node XY is safe because ...').

What do you think?

Following the thought in #20 I created a patch with comments for each hash line.

I thought there was a usable method to extract values from a "one value per line, in the format key|value" format.
Unfortunately extractAllowedValues() and allowedValuesString() in Drupal\options\Plugin\Field\FieldType\ListItemBase are static - so I had to copy some of the code from there.

Added interdiff to #19 and new patch.

What do you think?

Hi Greggles or anyone, hope you will see this after 2 years. I tried #16 and it worked, but some of my pages are using URL parameters to just show content for a specific month (for example, ....../agendas-minutes?date=2019-09). My embedded code is in the sidebar, so it appears to multiple URLs creating hashes. So every time I use add my hash, another one pops up. Is there Anyway around this?

Thank you!

If this is still needed it will need to be rerolled

Attempting to test but could use some instructions for how it should work.

What about this/

Updated MR to display hushed findings.

I have tested this and is functions as expected and without errors.

Tested on 10.1.x-dev with a standard install
module drupal/security_review:2.0.x-dev@dev
module branch 2678112-make-fields-check

Test procedure:
Create new Basic Page
Set body wysiwyg to Full Html
add add script tag and save published version of node
goto security_review and run testsa
see failure for that node
goto details of the failure and capture the hash value
goto security_review config page and add "$HASH|reason" as a value to "Dangerous tags in content exclude list" field
rerun security_review test
see success for "Dangerous tags in content"
got to details and see the node still listed, but underneath the "Hushed Findings" header

Thanks!

Thank you for this fix!