Fix CVE-2022-4993 stop routing foreign text into the Locale::Maketext format

[robrwo]

The first argument to add_error is the Locale::Maketext FORMAT: bracket groups in it are compiled into method-dispatch code. Three kinds of text that FormHandler did not author reach that position, all of them carrying submitted request data:

• _apply_actions traps warnings into $error_message (Validate.pm, the $SIG{WARN} handler). A warning survives a SUCCESSFUL action, so an ordinary numeric transform on a text field turns Argument "[sprintf,%2000000000d,0]" isn't numeric into the format -- Perl quotes the value verbatim, so the group is well formed and reaches CORE::sprintf with an attacker-chosen width (~GB allocation).
• a type constraint's failure message. Moose renders the rejected value with Devel::PartialDump when it can load it, Type::Tiny always uses its own dumper, and both render a reference in bracket-and-comma form -- so on any field with apply => [ Str ], two same-named request parameters put [ "a", "b" ] in the format and maketext croaks, which add_error re-dies: an unhandled 500 with no payload at all.
• exceptions from a coercion or transform.
• a date parser's error message. Field/Date.pm passes $strp->errmsg || $@ -- DateTime::Format::Strptime's own text -- straight into the format position. DTFS 1.80 answers a rejected value with the fixed string "Your datetime does not match your pattern.", so there is no reachable payload through it today; that is a property of the current version of a separate distribution rather than of this code, and the || $@ fallback is a second channel that was not exercised. Escaped for the same reason as the others.

Escape the bracket-notation metacharacters in all four before they are used as a format. Tilde is Locale::Maketext's escape, and text with no brackets is returned unchanged, so lexicon lookups and translated type-constraint messages are byte-identical to before.

Escape the bracket-notation metacharacters in all four before they are used as a format. Tilde is Locale::Maketext's escape, and text with no brackets is returned unchanged, so lexicon lookups and translated type-constraint messages are byte-identical to before.

Also: add_error derefs an arrayref first argument into (template, @Args). That spelling is the same list-or-arrayref convenience idiom as add_element_class and friends; it is not documented for add_error, and an instrumented run of the distribution's own suite (150 files, 1491 tests) never reaches the branch. What does reach it is request data -- $field->add_error($field->value) where the request parser folded a duplicate parameter into an arrayref puts submitted text in element 0. Since no message the library raises arrives in that shape, treat an arrayref argument as a value: keep the deref, render element 0 literally. A caller who wants a compiled template passes it as a plain list, $field->add_error($template, @args), which is the documented spelling and is unchanged.

One case cannot be fixed here: an application that concatenates the value into its own message, add_error("The value '" . $field->value . "' is not allowed"), is indistinguishable from a legitimate template, so the add_error POD now documents the hazard and the inert-argument idiom.

Behaviour trade-offs -- the only output changes outside the attack cases:

• a custom type constraint whose own message block uses bracket notation (message { 'Try [quant,1,thing]' }) now renders that literally. Such a block receives the rejected value and can interpolate it, so escaping it is the safe default; a maintainer who would rather keep those compiled can exempt the has_message branch specifically.
• an application calling the undocumented arrayref spelling with a template that uses bracket notation, add_error([ 'Try [quant,_1,thing]', 3 ]), now renders it literally; the list spelling of the same call still compiles.

FormHandler's own message templates, and application templates passed to add_error together with their arguments, are unaffected.

Verified against the 0.40068 test suite: 150 files, 1491 tests, PASS both before and after. A before/after table of rendered error messages (maxlength, minlength, required, invalid select value, integer range, duplicate-parameter arrays, application template with arguments, plain and bracketed type messages, plain and bracketed warnings and exceptions) is byte-identical except the lines above.

[robrwo]

@abraxxa @zby For future reference, who is maintaining this module, and who should the security contact be?

[abraxxa]

We should probably link to https://metacpan.org/dist/Locale-Maketext/view/lib/Locale/Maketext.pod#BRACKET-NOTATION-SECURITY in the pod.

Can we define an (empty? minimal?) allowlist to improve security more than escaping unknown input?

[abraxxa]

I don't understand this sentence, is it a typo?

[robrwo]

No, it makes sense to me.

The format argument is assumed to be bracket-notation-template and compiled.

[abraxxa]

I was referring to Messages FormHandler itself authors are templates on purpose, but three kinds of text reaching _apply_actions are not ours and do embed request data:.

[robrwo]

We should probably link to https://metacpan.org/dist/Locale-Maketext/view/lib/Locale/Maketext.pod#BRACKET-NOTATION-SECURITY in the pod.

Can we define an (empty? minimal?) allowlist to improve security more than escaping unknown input?

We can try that and see if it works. I'm unsure part of the problem is that it may not work as well as it should.

[abraxxa]

We could also just pass the error message always as second argument and '[_1]' as first one, which wouldn't require escaping the error messages.

[robrwo]

Keep in mind that the code was mostly written by Claude. So there is likely a much better way of doing this.

[abraxxa]

Can you please add tests that prove to be not vulnerable? Thanks!

[robrwo]

To be fair, I am unhappy with the possible solutions. I like the idea of passing '[_1]' plus the error better, but caching of values breaks that.