-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256
On Sun, Jul 26, 2020 at 11:12:14PM +0200, Wojtek Porczyk wrote:
> On Sat, Jul 25, 2020 at 10:46:40AM +, WillyPillow wrote:
> > On Saturday, July 25, 2020 12:18 AM, Wojtek Porczyk
> > wrote:
> > > On Thu, Jul 23, 2020 at 05:45:56PM +, WillyPillow wrote:
> > >
> >
> > > > One issue is that from the qrexec client side it is basically
> > > > impossible to
> > > > distinguish between the two. (Consider the case where a field contains
> > > > `xxx\\na:b:c`.)
> > >
> >
> > > If there are more colons that there are supposed to be, there is no need
> > > to
> > > distinguish anything anymore, just error out for "malformed input" or
> > > something.
> > >
> >
> > > In Python I like to do it with tuple assingment:
> > >
> >
> > > try:
> > > field1, field2, field3 = untrusted_line.split(':')
> > > except TypeError:
> > > raise ParseError('error message')
> >
> > By "distinguish between the two" I meant correct input and malformed input.
> > Sorry for the confusion.
> >
> > The point I was trying to make in that example was that when there are
> > colons
> > *and* newlines, there could be cases like the following:
> >
> > ```
> > field1 = 'a'
> > field2 = 'b'
> > field3 = 'c\nd:e:f'
> > encoded = ':'.join([field1, field2, field3]) # a single entry
> > encoded_list = '\n'.join([encoded] * 10) # simulate multiple entries
> > # on the other side...
> > decoded_list = encoded_list.split('\n') # becomes 20 rows
> > for untrusted_line in decoded_list:
> > field1, field2, field3 = untrusted_line.split(':') # this does not
> > error out
> > ```
>
> - Yes, this breaks something and is not distinguishable from template manager.
> - No, it's not malformed, because it conforms to specification.
> - It's OK from security perspective: You cannot cause installation of anything
> the user didn't explicitly mean (by choosing an item on a list and/or
> accepting a signing key).
>
> To reiterate: in optimal case this should be avoided from the sending side,
> but it't not a very high priority. It would be also OK if the tool and
> specification did something like "only the first line with ':' skipped".
>
> The worst case is something can be shown on the list of available templates
> that actually cannot be installed.
Additionally, the canonical way to build template rpms (that are then
included in the repo) is linux-template-builder (or perhaps some other
template builder in some future) and we can easily make sure all the
(benign) package metadata matches the (possibly limited) specification of the
qvm-template.
> > > It's as simple as that. The big advantage is that there aren't many ways
> > > to do
> > > something wrong.
> > >
> >
> > > > Security-wise, this is unlikely to cause issues as an entity that can
> > > > do this
> > > > can probably modify the repo contents directly.
> > >
> >
> > > The point is, we don't know. The repo content is untrusted, and yes,
> > > attacker
> > > can modify it. What counts is signature on RPM.
> > >
> >
> > > > However, if the repo, by accident, does contain packages with, say,
> > > > colons in
> > > > summaries, it may be an issue usability-wise as it's hard to give
> > > > meaningful
> > > > error messages when things break.
> > >
> >
> > > "Malformed input" is OK. If we break loudly, template maintainers (the
> > > honest
> > > among them) won't publish such summary, because it will break.
> >
> > The issue is that it's not always possible to detect such errors, as
> > elaborated
> > above. Of course, as long as we keep this "wart" explicit, I'm okay with
> > having
> > this as an "undefined behavior". (After all, the repo listing *is*
> > considered
> > untrusted anyway.)
>
> Sure.
>
> > > > There's also the original issue with descriptions (assuming that we
> > > > don't omit
> > > > them), which contains newlines a lot of the time.
> > > > That being said, if we treat such errors as "repo errors" and leave to
> > > > the repo
> > > > maintainers to ensure that the fields follow a certain format, then we
> > > > can just
> > > > use a special character for the separator [5] and ban the character
> > > > from the
> > > > fields.
> > >
> >
> > > Yes, and IIUC the current proposal is to have ':' as that special
> > > character.
> > > Am I missing something?
> >
> > The reason I'm bringing this up is twofold:
> >
> > 1. The idea of having the repo maintainer ensure such constraints have not
> > been
> >explicitly brought up before AFAIK.
> > 2. I'm unsure whether `:` is the best choice for this, as it's not an
> > uncommon
> >character, and banning it might be undesirable. Something like ASCII
> > 28-31
> >are possible alternatives.
>
> I think this should be one of the printable characters, so the output is easy
> to read and audit. Though ':' might well not be the best character, but there
> are others like '|' or even '!' (and I'd argue banning '!' from
