🇫🇷France @prudloff

I merged the latest 11.x and deprecated the gzip properties instead of removing them.

prudloff → made their first commit to this issue’s fork.

yesct → credited prudloff → .

I'm not sure how to deprecate an event subscriber since https://www.drupal.org/node/3357408 → .
It seems there is no way to prevent the event subscriber from handling events without removing it entirely (or disabling autowiring for the entire module).

Yes looking at those headers is helpful but it can be tricky to test if you have other parts of the page that prevent caching.

Another way to test this is to save your view. Without the patch, the view display will have this config:

    cache_metadata:
      max-age: 0

With the patch, it should turn to -1 (if your view is cacheable of course).

prudloff → created an issue.

I think there is also an expanded key that is missing in optional keys.

I agree this could be useful. This is kind of a power-user feature and content editors often get confused by it.

prudloff → created an issue.

I can reproduce the problem with your steps.
I suppose we have a difference in our environments.

Do you have private files enabled? It seems I don't have this problem if the screenshot is uploaded to the public file system.
You could try adding this at the beginning of steps to reproduce:
1. Add $settings['file_private_path'] = '/foo/'; in settings.php.
2. Rebuild cache (drush cr).
3. Browse to /config/media/file-system and set the default scheme to private.
4. Rebuild cache (drush cr).

prudloff → created an issue.

I merged the latest 11.x.

If you are 100% sure you can trust your headers, you can add something like this to remove the error:

$settings['trusted_host_patterns'] = ['.*'];

prudloff → created an issue.

Also this will need tests.

I agree this could be useful, it is sometimes hard to debug reverse proxy problems in production.
But I feel like the MR is trying to do too many different things and adds a lot of information. I'm not sure everything is relevant to everyone and it will make the MR harder to review.
Displaying the host, IP address, protocol and port detected by Drupal (using the methods on the Request object) would be a great start.

xjm → credited prudloff → .

xjm → credited prudloff → .

I just found a vulnerability in a contrib module related to this, so I agree this is still a concern.

I wonder how many use cases would break if we called Xss::filterAdmin() in t().

I applied the suggestion.

prudloff → made their first commit to this issue’s fork.

At least part of my problem in #10 was related to this ✨ JSON API integration Active so in case anyone else has the problem : the "administer crop" permission is need to get crop entities from the JSON API.

prudloff → created an issue.

prudloff → created an issue.

prudloff → created an issue.

prudloff → created an issue.

greggles → credited prudloff → .

I rebased the MR again.

The patch from #9 blindly replaces Xss::filter() calls with a render array, even in places where the expected type is a string.
For example:

-    $safe_string->string = Html::normalize(Xss::filter($string, static::allowedTags()));
+    $safe_string->string = Html::normalize(['#markup' => $string, '#allowed_tags' => static::allowedTags()]);

This would definitely not work.

Which is why I did not use it as basis for the MR and started from scratch.

xjm → credited prudloff → .

damienmckenna → credited prudloff → .

Not sure I follow why Markup::create(Xss::filterAdmin($element['#value'])); is no longer needed.

#markup already calls Xss::filterAdmin() when it is rendered.

I think this is still relevant.
I opened a MR based on the latest patch.

prudloff → made their first commit to this issue’s fork.

prudloff → made their first commit to this issue’s fork.

I added nl2br back.

I fixed the problem report by the bot (but I think it checked the wrong MR?).

prudloff → made their first commit to this issue’s fork.

prudloff → made their first commit to this issue’s fork.

prudloff → made their first commit to this issue’s fork.

I rebased the MR.

I merged the latest 11.x and applied suggestions.

I rebased the MR.

$computed_hash is not user-input.

Well I think this is were we disagree on the definition of user-input. IMHO a hash calculated from user input should be treated as user input here but I could be wrong.
To be fair, the PHP doc does not give a lot of examples and the only example is not very similar to our use cases.
I opened an issue about improving the examples: https://github.com/php/doc-en/issues/4771

I cannot see in which way they could rewrite the code to be dependent from the arguments order and still be constant-time code.

I'm not fluent in C but my understanding is that it matters in algorithms that can handle hashes with a different length (which hash_equals() does not support currently but could add in the future) because it could stop the comparison early if the user string and known string are inverted.
I have not checked if there a places in core where we compare hashes that can have a different length.

The order does not depend on which variable is pre-calculated or not, it depends on which variable the attacker is trying to brute force.

In the PHP doc example, the attacker is trying to brute force a valid signature, so the signature submitted by the user has to be second (and it is compared with a signature calculated from the secret key).
In PhpassHashedPasswordBase::check(), the attacker is trying to guess the password so the hash computed from the sent password has to be second.

It cannot be a value stored in the database.

It can totally be a value stored in database. The known string is the correct value that does not vary per request, it is either stored pre-calculated or calculated from known values like a secret key.
When the PHP doc says the known string "must be kept secret" it does not mean it can't be stored, it means it is the sensitive string that an attacker would be trying to get with a timing attack.
The user string is the hash that varies with each brute force request.

Timing attacks work by sending a lot of different values and checking if the time it takes the server to respond vary.
For example when trying to brute force a password, the website will usually have a hash of the password stored in the database and will hash the user-supplied password and compare it to the hash stored in database.
In that scenario, the known string is the hash stored in the database and the user string is the hash calculated from the password sent by the user.

Not sure about the target="_blank" but I agree that every link should use HTTPS.
The code still contains non-HTTPS links to Wikipedia for example:

core/misc/jquery.form.js:                                * "Same origin policy" http://en.wikipedia.org/wiki/Same_origin_policy
core/modules/basic_auth/src/Hook/BasicAuthHooks.php:        $output .= '<p>' . $this->t('The HTTP Basic Authentication module supplies an <a href="http://en.wikipedia.org/wiki/Basic_access_authentication">HTTP Basic authentication</a> provider for web service requests. This authentication provider authenticates requests using the HTTP Basic Authentication username and password, as an alternative to using Drupal\'s standard cookie-based authentication system. It is only useful if your site provides web services configured to use this type of authentication (for instance, the <a href=":rest_help">RESTful Web Services module</a>). For more information, see the <a href=":hba_do">online documentation for the HTTP Basic Authentication module</a>.', [
core/modules/breakpoint/help_topics/breakpoint.overview.html.twig:<p>{% trans %}Resolution multipliers are a measure of the viewport's device resolution, defined to be the ratio between the physical pixel size of the active device and the <a href="http://en.wikipedia.org/wiki/Device_independent_pixel">device-independent pixel</a> size. The Breakpoint module defines multipliers of 1, 1.5, and 2; when defining breakpoints, modules and themes can define which multipliers apply to each breakpoint.{% endtrans %}</p>
core/modules/breakpoint/src/Hook/BreakpointHooks.php:        $output .= '<dd>' . $this->t('Resolution multipliers are a measure of the viewport\'s device resolution, defined to be the ratio between the physical pixel size of the active device and the <a href="http://en.wikipedia.org/wiki/Device_independent_pixel">device-independent pixel</a> size. The Breakpoint module defines multipliers of 1, 1.5, and 2; when defining breakpoints, modules and themes can define which multipliers apply to each breakpoint.') . '</dd>';
core/modules/datetime/src/Hook/DatetimeHooks.php:        $output .= '<dd>' . $this->t('Dates can be displayed using the <em>Plain</em> or the <em>Default</em> formatter. The <em>Plain</em> formatter displays the date in the <a href="http://en.wikipedia.org/wiki/ISO_8601">ISO 8601</a> format. If you choose the <em>Default</em> formatter, you can choose a format from a predefined list that can be managed on the <a href=":date_format_list">Date and time formats</a> page.', [
core/modules/datetime_range/src/Hook/DatetimeRangeHooks.php:        $output .= '<dd>' . $this->t('Dates can be displayed using the <em>Plain</em> or the <em>Default</em> formatter. The <em>Plain</em> formatter displays the date in the <a href="http://en.wikipedia.org/wiki/ISO_8601">ISO 8601</a> format. If you choose the <em>Default</em> formatter, you can choose a format from a predefined list that can be managed on the <a href=":date_format_list">Date and time formats</a> page.', [
core/modules/help/help_topics/core.web_services.html.twig:<p>{% trans %}<a href="http://en.wikipedia.org/wiki/Basic_access_authentication">HTTP Basic authentication</a> is a method for authenticating requests by sending a user name and password along with the request.{% endtrans %}</p>

I opened a MR based on the old patch.

prudloff → made their first commit to this issue’s fork.

Please see my comment in #14, $computed_hash is calculated from a user-provided password. $stored_hash comes from the database so is not user-provided.
User-provided in the context of timing attacks means provided in the HTTP request.

I think we could open feature requests to add missing config_override_warn features to core.

Would someone be able to summarize which config_override_warn features are not in core yet?

A PHP contributor confirmed that the order does not matter with the current implementation but it could change in the future: https://github.com/php/doc-en/issues/4767#issuecomment-3054071094

So I think we should try to use the correct order to be future-proof, so setting this back to needs review.
I also updated the summary to remove the point about checking the string length. I don't think leaking the hash length is really a problem and it would hard to prevent.

(I also added back the empty sections of the issue summary, it helps seeing what still needs to be done.)

I take that in the following code the $known_hash parameter should be $computed_hash, and the $user_string parameter should be $stored_hash.

The user string is the hash calculated from values supplied in the HTTP request.
In your example, $computed_hash is calculated from the password provided to be checked, so having it as the second parameter is correct.

prudloff → created an issue.

I think comments have been addressed.

Should we close this as wontfix then?

I opened an issue to clarify the PHP doc: https://github.com/php/doc-en/issues/4767

IMO, it does not generally make sense. For example, in Drupal, hashes are not always values users provide.

Well, if we use hash_equals() it's usually because the hash is provided by the user or calculated based on user-provided values.
If that's not the case, timing attacks are not possible and there is no reason to use hash_equals().

greggles → credited prudloff → .

According to this discussion the order was important in a previous implementation but not in the current one.

It that's really the case, should we try to get the PHP doc fixed?

prudloff → created an issue.

The screenshot does not say that it needs to be rebased. It just says that the latest automatic rebased failed (so I did a manual one) and that merging is blocked because someone asked for changes in a review. (I don't have permission to mark the threads as resolved.)
If you expand the merge checks, you can see that the conflict and rebase checks pass (cf. screenshot).

Are you sure? I already rebased it 3 days ago and now GitLab does not find any conflict.

I tested the MR and confirm it fixes the vulnerability.

Our logic is not exactly the same as SqlBase since we don't have a query object but it removes quite a lot of duplicate code so I think it's worth it.

prudloff → changed the visibility of the branch 3534674-extend-sqlbase to hidden.

prudloff → created an issue.

I removed support for batch_size since it is broken.
We can always add it back later if we find a way to make it work.

Could you please check if 🐛 fields() function broken Needs review fixes your problem?

I can reproduce the problem and confirm the patch fixes it, thanks!

I think we need a followup for the todo (remove deprecated code branch in UserPasswordResetForm).

I believe comments have been addressed but we still need the CR and the followup.

I did not have time to test the MR yet.
I want to merge 🐛 Fix CI errors Active first, then the CI results for this MR will be easier to read.

I merged the latest 11.x.

I merged the latest 11.x.

I rebased the MR.

I rebased the MR but I agree it is annoying having to rebase every time something changes in composer.lock.

prudloff → changed the visibility of the branch 11.x to hidden.

prudloff → created an issue.

The remaining phpstan errors in fields() will probably be fixed by 🐛 fields() function broken Needs review .

prudloff → created an issue.

This might be fixed by 🐛 fields() function broken Needs review .

xjm → credited prudloff → .

prudloff → created an issue.

prudloff → made their first commit to this issue’s fork.

prudloff → created an issue.

I'm not sure I understand what you are asking for here.
Do you need something to change in this module?