🇫🇷France @prudloff

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?

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

prudloff → created an issue.

Yes, I am still interested.

Oh right, I didn't see that the module does a string comparison with the footnote text.

The main problem with check_markup() is that it looses cache metadata or attached libraries generated by the rendered text (because it uses renderInIsolation()).
So if one of the filters used in the Footnote text format adds cache metadata or attached libraries, this metadata will be lost.
For example the "Caption images" filter adds the filter/caption CSS library to the page but if I enable this filter on the Footnote text format, this library is not added to the page.

A solution would be to do the same thing as check_markup but with render() instead:

    $build = [
      '#type' => 'processed_text',
      '#text' => $text,
      '#format' => 'footnote',
    ];
    \Drupal::service('renderer')->render($build);

This way cache metadata and assets correctly bubble to the page.

prudloff → created an issue.

prudloff → created an issue.

The check_markup() doc says:

this function should only be used when filtering text for use elsewhere than on a rendered HTML page. If this is part of an HTML page, then a renderable array with a #type 'processed_text' element should be used instead of this

So I think we should use something like this:

      'text' => [
        '#type' => 'processed_text',
        '#text' => $text,
        '#format' => 'footnote',
      ],
      'text_clean' => PlainTextOutput::renderFromHtml($text),

I don't think the format needs to be applied to text_clean since PlainTextOutput::renderFromHtml() removes every tag anyway.

poker10 → credited prudloff → .

greggles → credited prudloff → .

greggles → credited prudloff → .

greggles → credited prudloff → .

prudloff → created an issue.

prudloff → created an issue.

prudloff → created an issue.

I think an exception throw is the best way to notify the developer..

I agree but I wonder if it could be a breaking change if some custom code relies on passing something that is currently evaluated to true?

What's the point of having a second ResultPlural class parallel to the existing one?

This is explained in #56.

Not that both |raw and Markup bypass Drupal's XSS filter so the module should make sure the markup in this variable is safe: 🐛 XSS in footnote text Active

Is there any reason to truncate the hash in ImageStyle::getPathToken()?
Would using the whole hash cause problems?

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

A potential solution would be to add a warning for text format when user uses configuration that is known to enable self-XSS.

Could this be covered by ✨ Provide warning when trying to use dangerous HTML markup Needs work ?

prudloff → created an issue.

This module does something similar: https://www.drupal.org/project/protect_form_flood_control →

prudloff → created an issue.

I merged the latest 11.x

I merged the latest 11.x.

prudloff → created an issue.

prudloff → created an issue.

prudloff → created an issue.