-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A hangs close to ten minutes in qpdf #243
Comments
|
This issue was assigned CVE-2018-18020. |
|
This is a pretty heavily damaged PDF file, but I think qpdf is handling it in a reasonable way. @carnil For what it's worth, when I run the latest qpdf on this on my Linux VM running on my mac, it finishes in under a minute, uses a reasonable and constant amount of memory, and does not crash. When I run it under address sanitizer, it shows no incorrect memory access, unfreed objects, etc. I doubt a CVE is justified here. As far as I can tell, qpdf is robustly handling a very broken file. Is there a specific reason this has been assigned a CVE? |
|
I don't believe there is unbounded recursion here. While QPDFWriter calls unparseObject recursively, earlier CVE fixes bound the depth of objects that QPDF will create. As far as I can tell, that protection is working and is preventing QPDFWriter from unbounded recursion. In fact, the checking introduced in the fix to CVE-2018-9918 is actually triggered by this file. The message "ignoring excessively deeply nested data structure" appears twice in the output. |
FWIW, I do not know, I was only the messenger here (I noticed the CVE assignment while reviewing the feed update from MITRE referincing for qpdf this bug and the respective CVE assignment to CVE-2018-18020. I think this needs to be answered by the requestor, @Krace did you request the CVE? |
|
Yes,I think it maybe some problem,and I'll cancel that,I'm sorry
carnil <notifications@github.com>于2018年10月9日 周二上午1:10写道:
… @jberkenbilt <https://github.com/jberkenbilt>
This is a pretty heavily damaged PDF file, but I think qpdf is handling it
in a reasonable way.
@carnil <https://github.com/carnil> For what it's worth, when I run the
latest qpdf on this on my Linux VM running on my mac, it finishes in under
a minute, uses a reasonable and constant amount of memory, and does not
crash. When I run it under address sanitizer, it shows no incorrect memory
access, unfreed objects, etc. I doubt a CVE is justified here. As far as I
can tell, qpdf is robustly handling a very broken file. Is there a specific
reason this has been assigned a CVE?
FWIW, I do not know, I was only the messenger here (I noticed the CVE
assignment while reviewing the feed update from MITRE referincing for qpdf
this bug and the respective CVE assignment to CVE-2018-18020. I think this
needs to be answered by the requestor, @Krace <https://github.com/Krace>
did you request the CVE?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#243 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/Afb_gqsqM_z6BolGWHsKp_5T-ETZPJ7vks5ui4cigaJpZM4XLQKJ>
.
|
|
@Krace No problem, thanks for your dilligence. I'll go ahead and close this issue. I'm glad to see that past fixes are working. |
|
Hello,I run this on another machine Centos7.0 ,it still have a hangs about ten minutes. |
|
On the ubuntu 14.04 i686 ,it also hangs for almost 3mins rather less than 1min. |
|
I think the cpu cores and memory maybe have some influence. |
|
Okay, let's leave this open, and I can see if I can track down why it performs so badly. I'll leave it to others' judgment as to whether there should be a CVE for this. There are some things inherent to PDF that, in the absence of arbitary caps, will cause any PDF interpreter to behave in certain ways. Clearly though qpdf is taking a longer time to reject this file than other viewers. I'm not sure whether it's because it's giving up more slowly or whether I've implemented something in an inefficient way. It's also possible that my arbitrary cap of 500 levels of nesting is much larger than it should be. It's hard to imagine any legitimate PDF file that would require more than maybe 10 or 15 levels. Maybe 100 would be a better cap that would make this more tolerable, or maybe there's just something dumb in the error handling code. I would agree that RAM and CPU should determine the performance, so I'm confused as to why my linux VM running on a mac laptop should outperform 16 cores and 32 GB of RAM. I don't think old dependencies would matter. Maybe CentOS compiles things with memory guards that have a higher overhead or something. I'm not going to treat this as urgent unless there's a good case as to why I should, but I will get to it when I can. |
|
thanks for your relpy,I will read the code carefully later to find the
reason.
When I find anything interesting I will inform you.
:P
Jay Berkenbilt <notifications@github.com>于2018年10月9日 周二下午9:52写道:
… Okay, let's leave this open, and I can see if I can track down why it
performs so badly. I'll leave it to others' judgment as to whether there
should be a CVE for this. There are some things inherent to PDF that, in
the absence of arbitary caps, will cause any PDF interpreter to behave in
certain ways. Clearly though qpdf is taking a longer time to reject this
file than other viewers. I'm not sure whether it's because it's giving up
more slowly or whether I've implemented something in an inefficient way.
It's also possible that my arbitrary cap of 500 levels of nesting is much
larger than it should be. It's hard to imagine any legitimate PDF file that
would require more than maybe 10 or 15 levels. Maybe 100 would be a better
cap that would make this more tolerable, or maybe there's just something
dumb in the error handling code. I would agree that RAM and CPU should
determine the performance, so I'm confused as to why my linux VM running on
a mac laptop should outperform 16 cores and 32 GB of RAM. I don't think old
dependencies would matter. Maybe CentOS compiles things with memory guards
that have a higher overhead or something.
I'm not going to treat this as urgent unless there's a good case as to why
I should, but I will get to it when I can.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#243 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/Afb_giyn0csmYRdEwcW9JMjAMPmik_Juks5ujKn_gaJpZM4XLQKJ>
.
|
|
I've been look at this a bit, and I think it's just inherent slowness of the way I use STL combined with the pathological nature of this file. qpdf's performance is good but not superb...mostly I have focused on ease of maintainability and robustness over speed. I'm going to leave this open, but I don't think I'm going to try to work on a fix. There doesn't seem to be any specific bug here. This would have to be addressed in the context of an overall effort to improve qpdf's performance. |
|
After recent fixes, this file no longer hangs for a long time. I recently added code to qpdf (not yet released, but it will be in version 9) that bails out if it encounters too many consecutive parsing errors. Thanks for the report. |
|
Hi @jberkenbilt , could you please point out where the fix was made ? (what commit) |
|
Hi @NicoleG25 . How important is it to do that? It would be possible to find the commit, but it would require using bisect. I could do it, but it would take time and effort since I didn't specifically target this particular issue. The way it would work would be to use If you can explain why you want to know this, it would help me determine whether it's worth it for me to spend the time doing this. Alternatively, anyone can do this analysis with git bisect. At each step, you could rebuild with |
|
Since @NicoleG25 and I both frequently ask developers this, I will give my input. There are times where a vendor has implemented a library and made significant changes to it. Instead of just integrating a new version, they do one-off code patches based on your fixing commits. In other cases, a vendor may have integrated the library as-is, but won't upgrade it without verifying the need to do so. In those cases, a vendor may not upgrade just because there was a 'security fix', as the functionality that was found vulnerable wasn't implemented in their integration, or it was and there is no user code path to reach a vulnerable function. So the ability to independently evaluate a fix for multiple reasons leads me to ask for the fixing commit often times. For many projects, issues are closed out in the PR with the fixing commit, or even linking to the fixing PR that has the commit that was merged into master is all we need. That said, this project and several others 'enjoy' cases where an issue is fixed before it is fully evaluated by other code changes, and I know it is a pain in the ass to find the actual fixing commit. So I certainly appreciate the time involved and your consideration in doing so. For my company's purpose, it is very helpful to have it, but not mandatory. Thanks! |
|
@attritionorg @NicoleG25 You'll notice that I almost always close issues with commits that fix them, and I think it's a good practice. In this particular case, I was fixing a series of issues, and when I got to this one, it was no longer an issue. I don't know which commit fixed it. Most likely it was similar to a different issue that was fixed by a nearby commit. Do a Also, I have been a debian developer for a long time and was frequently in the business of backporting security patches, so I definitely get what you are saying and fully appreciate this. Obviously you fully understand the issue and the situation, as do I. This project is mostly a hobby for me though, and since anyone can do the git bisect to figure this out, I'm not inclined to spend my time doing it. But please understand it's not because I don't appreciate the value. It's just because there are only so many hours in a day. :-) Looking at the ChangeLog, from around June 13 until around June 22, 2019, I took time off work to do a full integration of qpdf with Google's OSS-Fuzz. There are very large changes including some ABI changes, hence the next release being 9.0.0. During that week, I went through and fixed many issues found by fuzzers, and I also did a bit cleanup on integer type conversions. After fixing those issues, I rechecked other open issues of similar nature and was pleased to find that most were fixed. In this particular instance, I can say the fix was most likely one of the commits I did during that period, and I can also say that it would be difficult to backport the changes in isolation. Hopefully this helps. |
|
Hi @jberkenbilt , thank you for your input and reply. Thanks in advance ! |
|
@NicoleG25 The vulnerable file is linked in the issue at the top in the original report. Let me know if you run into trouble, and I'll jump in and help out. |
|
Hi @jberkenbilt ! Thanks in advance ! |
|
The commit that fixed this was cf469d7. |
|
@NicoleG25 forgot to tag you -- see previous comment. It's cf469d7 |
|
Thanks @NicoleG25 for digging up that commit, and @jberkenbilt for the discussion and confirmation! |
Thank you @jberkenbilt for the confirmation ! :) |
hi,I find something maybe wrong in the newest qpdf.
the poc file will cause the program to be hanged about ten minutes.
Maybe this is a bug or feature?
poc.pdf
and I found that it maybe caused by the unparseObject in libqpdf/PDFWriter.cc,here are some backtrace:
Looking forward to you reply,thx : )
The text was updated successfully, but these errors were encountered: