search

OpenPrinting / cups

OpenPrinting CUPS Sources
https://openprinting.github.io/cups
Apache License 2.0
1k stars 178 forks source link

Fix driverless printing on Pantum M7300FDW #826

Closed alexpevzner closed 9 months ago

alexpevzner commented 9 months ago

According to RFC 8010, attributes within collection must not have name. Instead, name must be encoded as value of special dedicated attribute, IPP_TAG_MEMBERNAME

Unfortunately, some devices violate this specification and encode collection member names as attribute names.

Pantum M7300FDW is know to have this bug in firmware.

So if we have not seen IPP_TAG_MEMBERNAME while decoding the current message, we allow sender to violate this rule and threat attribute name as member name.

It fixes driverless printing on Pantum M7300FDW.

Please note, this change breaks bad_collection test in the testipp.c, because now it decodes. So I've slightly modified that test, adding IPP_TAG_MEMBERNAME to the first attribute in the collection - it disactivates the workaround, making sure that bad_collection will not be decoded, as expected.

I've also added to the test a real-life example of Pantum M7300FDW response to the Get-Printer-Attributes request which now decodes.

The similar patches already added to https://github.com/OpenPrinting/goipp and https://github.com/OpenPrinting/ipp-usb so with all this stuff taken together, the device becomes fully-functional in the driverless mode (printing/scanning, network/USB, things just work).

alexpevzner commented 9 months ago

@michaelrsweet, @tillkamppeter, please review when time allows.

@zdohnal, JFYI.

michaelrsweet commented 9 months ago

NO.

We cannot support broken printers like this without exposing CUPS to known security attacks. The code enforces the proper syntax for a reason, and we will not break CUPS for the sake of this printer.

alexpevzner commented 9 months ago

Hi @michaelrsweet,

Frankly speaking, I'm a little concerned about how quickly and abruptly this decision was made. Without discussion, without anything...

Let's be practical. Pantum printers become popular across the world. It is not some kind of exotic, unknown and rarely used hardware.

Yes, this is a broken printer firmware. However, we already has a common practice to support broken firmware where possible. CUPS quirks for USB devices, ipp-usb, sane-airscan contain a lot of kludges to support broken hardware. It doesn't make us, engineers, happy, but it makes our users happy.

After all, this is consistent with the Postel's principle to "be conservative in what you send, be liberal in what you accept".

And honestly speaking, I don't see any security risk introduced here. I understand that syntax is enforced purposely and would be glad to somehow limit the applicability of this relaxation only to these broken Pantum devices, but at this place there is no indication what device we are speaking with.

Also, I'd like to receive some feedback from distro maintainers. @zdohnal, @tillkamppeter, WDYT?

michaelrsweet commented 9 months ago

We have already fixed multiple security vulnerabilities in this code, specifically to do with invalid IPP messages from printers. It causes erratic behaviour, crashes, and disclosure of information. So no matter how “popular” a printer might be, we are not allowing obviously bad data to open up security holes in CUPS. If it is actually so popular, the vendor can fix their printer firmware!