Re: Bug#1064454: debian-policy: Restrict deb822 field names more

Guillem Jover:
> Hi!
> 
> On Thu, 2024-02-22 at 12:10:06 +0100, Niels Thykier wrote:
>> Package: debian-policy
>> Severity: wishlist
> 
>> While the parser is technically correct given the Policy description above,
>> I cannot see any case where this is a desirable outcome. Instead, I think we
>> should classify this as a syntax error such that users are instructed to
>> indent `${shlib:Depends}` token.
> 
> Currently both the dpkg C and Perl parsers are extremely lax (even more
> than what the deb822(5) manual page or the Debian Policy permit! No matter
> the outcome of this discussion, I'm going to make both these parsers at
> least as strict as the current dpkg documentation.
> 
> [...]
> 
> Perhaps the allowed syntax should even be defined by context, so that
> the initial underscores are only allowed for debconf template files.
> 
> (This would be made strict for the Perl parsers for the build tools,
> strict for the C parser in charge of building packages or installing
> them anew, and lax with warnings for existing installations to avoid
> bricking potential current users with such field names.)
> 

I would like to avoid "context" based rules here. Most "deb822" parsers 
are not context aware other than "maybe" allowing GPG signatures. I 
think only `dpkg` and `lintian` supports it. For lintian, I think it is 
limited to `debconf` templates (which just have lintian accept 
"space-only continuation lines" in addition to " ."). Many parsers are 
just ad-hoc regex based parsers.

For me, adding context awareness would complicate rather than simplify.

> But see below.
> 
>> A simple fix would be to forbid `$` at the start of the field. Though, I
>> think at this point it would make sense to prune more special characters
>> from the list like `%&{}[]()/\\<>|$` from anywhere in the field. Note that
>> we definitely need to keep `_` as it is used in debconf template files for
>> translations. Both single and double underscore is used here, though they
>> always come first in the field name.
>>
>> The reason why I want a tighter bound is that currently, the following
>> things are also considered valid field names:
>>
>> [...]
>>
>>
>> In all cases, I see no value to allowing these absurd field names and only
>> potential downsides.
> 
> I was pondering about this and while I'm in general a strong proponent
> of rather strict parsing, to avoid garbage in garbage out, that
> external users would otherwise have to then protect against and deal with.
> In this case I could see how making the parsing overly strict could stymie
> potential innovation, like when debconf started using «_» to mark some
> fields in a special way. I mean we can always go around and carve out new
> exceptions, but that requires way more effort, and then one needs to
> justify those new uses, and convince others, which might be hard (once
> the allowed format is very strict)! We also do not have visibility
> over privately used field names. And there's the problem that allowing
> new syntax might require one release cycle to be able to reintroduce
> it, if the previous dpkg/apt need to be able to support it.
> 

In my view, any innovation at that level should involve moving away from 
deb822 as the source format and therefore, this is a non-issue for me. 
If needed be, it can always be translated into deb822 for `dpkg`, `dak`, 
etc. This is just to make it explicit that I am not assigning this part 
of your argument as much weight as you might have hoped.

This is also why I focus on what we are using today rather than what 
someone might think of using in the future.

Additionally, even if we had restricted `deb822` from starting fields 
with `_`, we would not have prevented `debconf` translation feature. 
They would have had plenty of options like a different prefix.

> Perhaps, as you mention, a more lenient parser that just restricts field
> start (from at least anything that can easily appear in a dependency field,
> for example) would accomplish most of the benefit, w/o blocking potential
> future directions? Say something like the following untested regex:
> 
>    ^[_]*[A-Za-z0-9]+[!-9;-~]+$
> 
> (Or perhaps if we are allowing bracket characters, then require them to
> be balanced, but that might be too much effort, if we are never going to
> make use of them.)
> 

I am against anything that can be interpreted as a path (a basename is 
unavoidable) or when unescaped in shell context will trigger some kind 
of shell specific thing. For me, that rules out:

"`ls`" -> Runs `ls`
"(ls)" -> Runs `ls` in a subshell
"foo;ls" -> Can cut a command in half and run ls.
"{foo,bar}" -> Can split an argument in half (command line argument
   injection)

The project has repeatedly showed that we throw deb822 field names 
around like they are not going to cause issues. There will be a next 
time if we do not stop this at its root. On the other hand, the project 
has not shown any need for any of these in any setting (innovation or 
otherwise).

I would additionally also restrict the use of `0x7F` (DEL) and I assume 
the regex is `ASCII` restricted (I don't want to deal with 
LRM/Left-to-Right marks, though I think policy also ASCII restricts 
field names, so that might have been implicit in your reply).

> Alternatively we could have hard and soft restriction, hard with
> something like the above regex producing errors, and soft like the
> first regex, which would only produce warnings (although that might
> still be enough of a deterrent that one would still need to update
> parsers anyway for new syntax).
> 

For me, that is unnecessary complexity. I want simplicity, not 
flexibility. Both as a deb822 consumer and as a (generic) deb822 parser 
writer.

Note, I am not against parsers supporting this an opt-in (I would not in 
the parser I am working on). So `dpkg-deb --nocheck` could switch to 
your "soft restriction" if you wanted to. But I am against it defining 
it in the specification for what *all* parsers should accept/support (in 
the "SHOULD"/"MUST" sense. I would not be happy but I could accept a MAY).

> But in any case, this latter regex would not allow for the exact same
> extensibility that debconf used at the time to prefix the field with a
> special character (although it very much preserves syntax freedom at
> the end), which perhaps is an indicator, that we should simply restrict
> it all, and if a new syntax is required we can always introduce it later
> on?
> 
> New syntax that comes to mind could be stuff like:
> 
>    Field-Name(something): value
>    !Field: value
> 
> Or things along these lines.
> 

Both examples could have side-effects when used unescaped shell, which 
has happened numerous times in the past and will happen again if prior 
experience is an indicator.

The closest thing I could see us doing in this vein would have been 
`Description[de]` or so. But then the project seems to have favored 
`Description-de` instead. Now `Description[de]` could have had some nice 
semantic properties in some cases, but I feel the ship has sailed on 
which convention the project uses.

>> But someone will eventually try to do this. We already received a bug report
>> against debhelper long ago about being able to do path traversal via
>> questionable package names. Largely, this was considered a non-issue because
>> dpkg in its default configuration would abort and debhelper has a "run
>> arbitrary code via the upstream build system" feature anyway.
> 
> Do you have a reference on hand to that report? Because if dpkg fails
> I'd assume that might be a side effect from something else, and it
> might make sense to make sure this is handled explicitly.
> 
> Thanks,
> Guillem

My use of `abort` here was a short hand for `stops the build` rather 
than `SIGABRT` (in the latter case, I would have informed you). I think 
it was `dpkg-deb` failing because it wanted `--nocheck` and the use-case 
for the "attack" (I think it was via `equivs`) did not enable the 
attacker to pass that.

It was reported privately as a security bug, so there is no public 
record. It  was also an "execute shell code" rather than path traversal 
as I remembered. Nevertheless, that attack relied on package name (field 
value) rather than field name. I was using it as an example that we take 
for granted that files are well behaved.
   For field names as path traversal, then I was probably thinking of 
CVE-2009-4013 in lintian 
(https://salsa.debian.org/lintian/lintian/-/commit/952b56c2b670a42bdb35608ee02e74593aa0d3d6).

Best regards,
Niels

PS: For the curious, the reported attack had been plugged in sid by 
`debhelper` no longer using shell as often for simple operations by the 
time it was reported. I still provided the following patches

a43b521162aa9d653d7ea36516839ef4484522c2..6718818559818fbd139fe79d393d9bceedea9595

to close the gap in case other helpers might still use shell and think 
certain field names or values would be safe to pass