Banning QNetworkAccessManager

Ben Cooksley bcooksley at kde.org
Fri Feb 21 22:20:16 GMT 2020


On Thu, Feb 20, 2020 at 9:57 PM Volker Krause <vkrause at kde.org> wrote:
>
> On Wednesday, 19 February 2020 10:04:11 CET Ben Cooksley wrote:
> > On Wed, Feb 19, 2020 at 9:30 PM Volker Krause <vkrause at kde.org> wrote:
> > > On Wednesday, 19 February 2020 08:05:01 CET Ben Cooksley wrote:
> > > > On Mon, Feb 3, 2020 at 7:42 AM Volker Krause <vkrause at kde.org> wrote:
> > > > > I agree on the problem of QNAM's default, see also
> > > > > https://conf.kde.org/en/
> > > > > akademy2019/public/events/135 on that subject.
> > > > >
> > > > > On Saturday, 1 February 2020 23:24:14 CET Ben Cooksley wrote:
> > > > > [...]
> > > > >
> > > > > > Prior to now, i've taken the approach of advertising that
> > > > > > QNetworkAccessManager is broken and needs a flag set within
> > > > > > applications when used, however unfortunately this seems to have
> > > > > > been
> > > > > > an ineffective approach and cases of broken behaviour continue to
> > > > > > appear (despite this brokeness being known about and advertised
> > > > > > since
> > > > > > back in the Qt 4.x series when this class was introduced)
> > > > > >
> > > > > > Therefore, given the Qt Project is unlikely to change their position
> > > > >
> > > > > > on this, I would like to propose the following:
> > > > > The Qt Project is actually in favor of changing at least the
> > > > > redirection
> > > > > default to exactly what we want it to be.
> > > > > https://codereview.qt-project.org/c/qt/qtbase/+/273175 has the change,
> > > > > and
> > > > > got approval both by Lars and one of the maintainers. It is merely
> > > > > stuck
> > > > > in dealing with the unit test fallout in Qt's CI that somebody has to
> > > > > take care of. Doesn't immediately help us of course, but claiming Qt
> > > > > is
> > > > > unwilling to change this is simply wrong.
> > > > >
> > > > > > 1) That effective immediately, QNetworkAccessManager and it's
> > > > > > children
> > > > > > classes be banned and prohibited within KDE software, to be enforced
> > > > > > by server side hooks.
> > > > > > 2) That we fork QNetworkAccessManager and the associated classes
> > > > > > within the appropriate Framework (to be determined), where the
> > > > > > defective behaviour can then be corrected.
> > > > >
> > > > > While this might solve the redirection problem, it brings us yet
> > > > > another
> > > > > then unmaintained HTTP implementation next to the one in KIO, which is
> > > > > a
> > > > > far bigger problem long term. We need less of those, not more.
> > > > >
> > > > > To address the problem of people not using the correct defaults, I did
> > > > > propose a much less extreme approach in the above mentioned talk at
> > > > > Akademy, namely having a KNetworkAccessManager as a sub-class of QNAM
> > > > > in
> > > > > a low-tier frameworks that essentially just enables redirects and
> > > > > HSTS.
> > > > > QNAM's implementation isn't the problem after all, the defaults are.
> > > > >
> > > > > Commit hooks warning about QNAM usage might still be a good idea, but
> > > > > as a
> > > > > warning only. Hard blocking QNAM-using code would block at least three
> > > > > repos I spend a lot of time on currently, none of which even talks to
> > > > > KDE
> > > > > servers.
> > > > >
> > > > > If you need to take more drastic measures against code not doing this
> > > > > correctly regardless, you can do that my dropping the server-side
> > > > > workarounds. Yes, that would break the affected application possibly,
> > > > > but
> > > > > at least it would not cause massive collateral damage for the people
> > > > > using this correctly.
> > > > >
> > > > > It would also help to know where specifically we have that problem, so
> > > > > we
> > > > > can actually solve it, and so we can figure out why we failed to fix
> > > > > this
> > > > > there earlier.
> > > >
> > > > Just bringing this up again - it seems we've not had much movement on
> > > > this aside from the Wiki page.
> > > >
> > > > Examining that Qt merge request, it not only is targeted at just Qt
> > > > 6.x, it also hasn't had any movement since the start of October 2019 -
> > > > 4 months ago.
> > > > Given that we are going to be on Qt 5.x for some time, that merge
> > > > request can't be considered to be the 'fix' for this issue.
> > >
> > > The targeting of Qt6 is due to this aiming at dev when that was still Qt
> > > 5.x, but you are right of course, somebody needs to do the work there to
> > > drive this to completion, no matter in which version it will actually
> > > land.
> >
> > Indeed.
> >
> > > > We need a solution that will ensure software released today doesn't
> > > > cause us pain in several years time, because as I illustrated in an
> > > > earlier email to this thread, software has a habit of remaining in use
> > > > for an extremely long amount of time (August 2014 being the release
> > > > date of the Marble versions in question hitting that old URL).
> > > >
> > > > The easiest path forward is to simply ban QNetworkAccessManager.
> > >
> > > That might be the easiest path for you, but certainly not for me, given
> > > two of the modules I work on most atm are using QNAM (not even to talk to
> > > KDE infrastructure), and would suddenly no longer be allowed to be hosted
> > > here?
> > Ideally they would move to whatever replacement we have for QNAM
> > should it be banned :)
> >
> > > Also, I have yet to see a suggestion for a viable alternative to QNAM. The
> > > initially proposed fork would mean trading the possibility of broken
> > > redirect handling for an unmaintained HTTP stack, I don't think that's a
> > > good deal for the security of our users.
> >
> > It isn't perfect yes.
> >
> > Sadly however the negative user experience created by countless broken
> > applications due to the Qt Project being uncooperative in this matter
> > for many years doesn't really give us much in the way of other
> > options.
> >
> > It doesn't help when Developers come to us asking for temporary
> > measures to be put in place to keep their applications alive as
> > they're getting bug reports about broken functionality. Saying no
> > saves us the effort of maintaining the fixes, but leaves applications
> > broken - which is not a fun position to be in as the one making the
> > decision.
> >
> > > Instead, how about the way we approached this for KUserFeedback? Before
> > > getting things deployed on KDE infrastructure for use by library or
> > > application code, sysadmins and developers review if the implementation is
> > > allowing easy and secure operation of the infrastructure, redirect
> > > handling
> > > being one part of this. In case of KUserFeedback you suggested protocol
> > > changes, they were implemented, and automatic tests for a number of
> > > possible redirect scenarios were added. Worked out fine IMHO, despite
> > > QNAM (the needed changes weren't due to missing redirect handling, but
> > > due to somewhat non- standard redirects on HTTP POST).
> >
> > Having a Sysadmin review an implementation that will be talking to a
> > server is in general always a good idea, even outside of this
> > particular usecase.
> > It certainly helps protect against this class of issues that is for sure.
> >
> > It isn't a perfect defense though, as if the review isn't requested
> > issues could slip past :(
>
> Right. However, as sysadmins are in control of what gets deployed on our
> infrastructure, you are in the perfect position to make sure such a review
> happens. I don't think anyone would have a problem with that being a hard
> requirement for getting server-side components or data deployed.

To a certain extent yes. For any kind of application software running
on our systems (which was the case with KUserFeedback) that is
definitely true.

When it comes to simple static files however, it is pretty straight
forward to add files to most of our websites - and people often have
self-service access to do this (by committing to a given Git
repository in many cases).

Most of the instances where we have encountered this have been with
static files sadly. Unless people are aware of the issues with
QNetworkAccessManager and know they need to ask review (even if it is
for a simple JSON file being retrieved) I fear it is quite possible
that this review would be missed.

Like the other options proposed (the Krazy/Clang AST checks, listing
it on our API to avoid pages, etc) this helps reduce the risk, but
still doesn't fully eliminate it sadly - especially not for existing
code.

If Qt is unwilling to change this, our other option of course is to
provide a patch to correct the brokenness in Qt and push it to
distributions, along with a very clear expectation that any Qt package
they provide has said patch to correct this fatally broken behaviour
included (and remove any distribution that fails to do so from our
pre-release access programs)

(Just to be clear, I don't want to be in the position of having to
make the decision as to whether applications running on end user
systems - that many have been released recently - have parts of their
functionality break ever again. Hence why this is basically war with
this particular set of classes within Qt)

>
> Regards,
> Volker

Cheers,
Ben

>
> > > There is a risk that the code breaks this again over time, independent of
> > > test coverage, but I'd say that's not sysadmin's problem then, it's due
> > > to application code not following the rules.
> > >
> > > This of course only helps with new infrastructure deployments, but so
> > > would a QNAM ban only help with new releases.
> > >
> > > Additionally, improved documentation, a possible KNAM and/or driving the
> > > QNAM changes upstream can still be done alongside this obviously.
> >
> > Something like KNAM would allow for a more refined ban yes (allowing
> > QNAM in the repository that hosts KNAM, but blocking it elsewhere)
> >
> > > Regards,
> > > Volker
> >
> > Cheers,
> > Ben
>




More information about the kde-core-devel mailing list