#22 CVE-2018-12982 - invalid memory read in PdfVariant::DelayedLoad()

[Mattia Rizzolo]

https://security-tracker.debian.org/tracker/CVE-2018-12982
PoC: https://bugzilla.redhat.com/show_bug.cgi?id=1595689

There exists one invalid memory read bug in PdfVariant::DelayedLoad() in base/PdfVariant.h in PoDoFo 0.9.6-rc1(the latest stable version). Remote attackers could leverage the this vulnerability to cause a denial-of-service via a crafted pdf file.

[Matthew Brincke]

AFAICS the bug isn't in PdfVariant::DelayedLoad() but its caller-caller (in the stack trace provided in the bug report to RedHat) PdfEncrypt::CreatePdfEncrypt(const PdfObject*) and there specifically lines 562 through 570. There the return values of the PdfDictionary::GetKey() calls need to be checked for NULL (sTmp could be used for this with type PdfVariant*) before they're used further. PdfVariant::DelayedLoad() cannot be fixed because a NULL value for the this pointer is AFAIK undefined behaviour.
An alternative way to fix this is to introduce a PdfVariant::MissingValue and have PdfDictionary::GetKey() return that when a key is not found, then the callers of PdfVariant::DelayedLoad() in PdfVariant can check for that before the call and and throw a PdfError exception which can be caught instead of the proposed NULL checks (to throw it again with a more specific error code of ePdfError_InvalidEncryptionDict which should be done anyway in the above, IIRC the dict keys queried are required). The biggest problem with this latter approach is that it would require extensive changes to any existing NULL checks, making it too invasive for a security update (IMHO).
If API could be added in a security update (can it?), the cleanest solution IMHO would be to add a throwing version PdfDictionary::GetKey() for direct keys (there's already one for indirect ones) and use that to get rid of the necessity for multiple NULL checks. Instead one try-catch would suffice as all those errors are handled the same (see the paragraph before in this post).

[Mattia Rizzolo]

Just answering this bit:

The biggest problem with this latter approach is that it would require extensive changes to any existing NULL checks, making it too invasive for a security update (IMHO).
If API could be added in a security update (can it?)

It could, but then no user would benefit from it as they would all require changes (if I understand correctly what you mean).
Also, you shouldn't worry too much about these details, after all it's a distributor job to evaluate the feasibility of a cherry pick. In general, if you want to be kind to us, simply please avoid changing the ABI: that's usually good enough to make cherry-picking of changes feasible.

[Matthew Brincke]

If API could be added in a security update (can it?)

It could, but then no user would benefit from it as they would all require changes (if I understand correctly what you mean).

If users diligently check for NULL returns from PdfDictionary::GetKey() already, no, they won't require changes with my last proposal, they are already protected against similar problems (to this in PoDoFo proper), and would just need the libpodofo update against this one, because using the throwing version would be completely optional: either check for NULL return or use the new throwing version, e.g. named PdfDictionary::MustGetKey().
Without either, AIUI there can't be a change to libpodofo which would benefit applications without changes (to those which do use NULL checks, in this case, because with MissingValue "key not found" no longer returns NULL, so this has to count as ABI change AFAIK).
Adding optional API (no behaviour or security changes with not using it) is not a "prohibiting cherry-picking" ABI change (because it would block downgrades only if applications would start to rely on it, and upgrades never, if the API isn't revoked, else same as downgrade), right (have I understood correctly)?

[Mattia Rizzolo]

I see what you mean.

You understood correctly my point about API/ABI, yes.

Alas, I don't think I'm able to provide more insight on whether that's the correct approach to fix this bug (if at this point it is actually a bug here, given what you say).

[Matthew Brincke]

I've committed an inline version of PdfDictionary::MustGetKey() so AFAIK there isn't an API or ABI change, and as it's used in the library it doesn't require a recompile of applications. I've tested it with the PoC to be fixed (svn r1948) so please close (not tested with OpenSSL 1.1 but there should be no impact).

[Matthew Brincke]

As nobody changed this in the meantime (maybe because I set myself as owner?), I'm closing it also (fixed since svn r1948).