Bug: OSS-Fuzz: Deny an unreasonably large header to avoid OOM (PR #26607)
Arthur Chan
This PR adds a guard to deny an unreasonably large header to avoid possible OOM in wxTarInputStream processing, discovered by OSS-Fuzz fuzzer (#26588).
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26607
Commit Summary
- 27e7176 Bug: OSS-Fuzz: Decline an unreasonably large header to avoid OOM
File Changes
(1 file)
- M src/common/tarstrm.cpp (8)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26607.patch
- https://github.com/wxWidgets/wxWidgets/pull/26607.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26607)Thanks, I don't know enough about (GNU?) TAR format to be sure if this limitation can be a problem in practice. Does anybody have any example tar files using extended header? I've tried a few of tarballs I had lying around but none of them uses it at all, so I couldn't see what could its reasonable size be like.
Maybe bump it up to 10MiB? This should be enough and still shouldn't be a problem to allocate on any modern system. We could also add a static wxTarInputStream function to set this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Arthur Chan
arthurscchan left a comment (wxWidgets/wxWidgets#26607)I don't think we care about legitimate tar files. But since the widget itself is meant to process tar stream and thus it be could possible to have an adversary just pass in a random stream claimed to be tar file (not necessary to be a real tar) but with extremely large extended header and cause the OOM.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26607)Err, we do care about legitimate tar files. In case I wasn't clear, my concern was that we could now start rejecting valid (legitimate) tar files which have extended header bigger than 1MiB. I don't know how likely is this to happen but it doesn't seem completely impossible, something like a Boost tarball can contain thousands of files with very long names.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Arthur Chan
arthurscchan left a comment (wxWidgets/wxWidgets#26607)er... true. I am now double checking about the standard for tar ball extended headers. Sorry for the rush conclusion.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Ian McInerney
imciner2 left a comment (wxWidgets/wxWidgets#26607)I looked in the POSIX 1.2001 standard, and there is nothing stating how long an extended header can be - it is basically left as a free field in terms of length.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26607)Thanks Ian, this confirms my suspicion that there is no universally acceptable limit here. But I still think adding wxTarInputStream::SetMaxExtendedHeaderSize() and setting it to 10MiB by default would be a reasonable thing to do.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Arthur Chan
arthurscchan left a comment (wxWidgets/wxWidgets#26607)Thanks for confirming. Do you want me help do the changes in this PR following the 10MB assumption you mentioned?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Miguel Gimenez
wh11204 left a comment (wxWidgets/wxWidgets#26607)I just want to remark that the last modification time field in the given header (bytes 136 to 147, Unix timestamp in octal) contains a '@', obviously not a valid octal digit. I do not know if the code uses this field at all, but it could possibly crash.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26607)Thanks for confirming. Do you want me help do the changes in this PR following the 10MB assumption you mentioned?
I am still not sure what to do about this, but the best idea I have remains to
- Restrict this to 10MiB (or any other arbitrary but pretty high number).
- Add a static function allowing to change this limit if the application ever really needs it.
If you could please do it, it would be great, of course. And if you can think of something better to do, it would be even better :-)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26607)I just want to remark that the last modification time field in the given header (bytes 136 to 147, Unix timestamp in octal) contains a '@', obviously not a valid octal digit. I do not know if the code uses this field at all, but it could possibly crash.
Sorry, I hardly know this code at all but if you think it's a bug, please open a separate issue for it, I don't think it's related to anything being discussed here (other than in the sense that the fuzzer should be able to find this bug as well).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Arthur Chan
@arthurscchan pushed 1 commit.
- 790d6ea Add static functions and restrain allocation
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Arthur Chan
arthurscchan left a comment (wxWidgets/wxWidgets#26607)I have updated the logic, now the logic rejects extended headers larger than a configurable cap (default 10 MiB, settable via wxTarInputStream::SetMaxExtendedHeaderSize()) instead of allocating the declared size up front. I also adds a check of size capped to 64-bit value before narrowing to size_t, and the allocation is clamped
to the bytes actually available on streams.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz requested changes on this pull request.
Thanks, this looks good but we need to document the new functions in interface/wx/tarstrm.h, could you please do this (please use @since 3.3.3 in their documentation to indicate that they're new)? TIA!
> @@ -909,8 +912,26 @@ bool wxTarInputStream::ReadExtendedHeader(wxTarHeaderRecords*& recs)
if (!recs)
recs = new wxTarHeaderRecords;
+ // read the declared size as 64-bit before narrowing, to avoid truncation
+ const wxTarNumber declared = m_hdr->GetOctal(TAR_SIZE);
+
+ // reject an unreasonably large extended header to avoid excessive allocation
+ if (declared < 0 || declared > (wxTarNumber)GetMaxExtendedHeaderSize()) {
+ wxLogWarning(_("invalid data in extended tar header"));
+ return false;
+ }
+
+ size_t len = (size_t)declared;
If we did this above, we could compare it with GetMax...Size() without a cast (< 0 would become unnecessary).
> @@ -909,8 +912,26 @@ bool wxTarInputStream::ReadExtendedHeader(wxTarHeaderRecords*& recs)
if (!recs)
recs = new wxTarHeaderRecords;
+ // read the declared size as 64-bit before narrowing, to avoid truncation
+ const wxTarNumber declared = m_hdr->GetOctal(TAR_SIZE);
+
+ // reject an unreasonably large extended header to avoid excessive allocation
+ if (declared < 0 || declared > (wxTarNumber)GetMaxExtendedHeaderSize()) {
Minor, but in wx coding style braces should be on their own line:
⬇️ Suggested change- if (declared < 0 || declared > (wxTarNumber)GetMaxExtendedHeaderSize()) {
+ if (declared < 0 || declared > (wxTarNumber)GetMaxExtendedHeaderSize())
+ {
> @@ -909,8 +912,26 @@ bool wxTarInputStream::ReadExtendedHeader(wxTarHeaderRecords*& recs)
if (!recs)
recs = new wxTarHeaderRecords;
+ // read the declared size as 64-bit before narrowing, to avoid truncation
+ const wxTarNumber declared = m_hdr->GetOctal(TAR_SIZE);
+
+ // reject an unreasonably large extended header to avoid excessive allocation
+ if (declared < 0 || declared > (wxTarNumber)GetMaxExtendedHeaderSize()) {
+ wxLogWarning(_("invalid data in extended tar header"));
This is an error and not a warning as the stream can't be used and I think it would be worth giving more details about the problem, i.e.:
⬇️ Suggested change- wxLogWarning(_("invalid data in extended tar header"));
+ wxLogError(_("Extended tar header size %zu is greater than maximum allowed."), len);
> @@ -909,8 +912,26 @@ bool wxTarInputStream::ReadExtendedHeader(wxTarHeaderRecords*& recs)
if (!recs)
recs = new wxTarHeaderRecords;
+ // read the declared size as 64-bit before narrowing, to avoid truncation
+ const wxTarNumber declared = m_hdr->GetOctal(TAR_SIZE);
+
+ // reject an unreasonably large extended header to avoid excessive allocation
+ if (declared < 0 || declared > (wxTarNumber)GetMaxExtendedHeaderSize()) {
+ wxLogWarning(_("invalid data in extended tar header"));
+ return false;
+ }
+
+ size_t len = (size_t)declared;
+
+ // don't allocate more than the stream can actually provide
Sorry, why do we need this? Allocating up to 10MiB shouldn't be a huge problem and we hopefully get an error if we read less data than expected (if we don't, we should fix this).
And if we really want to check for this, we should return an error if there is not enough data in the file as this means that it is corrupted, not allocate less.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Arthur Chan
@arthurscchan pushed 1 commit.
- 457278e Add static functions and restrain allocation
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Arthur Chan
@arthurscchan commented on this pull request.
> @@ -909,8 +912,26 @@ bool wxTarInputStream::ReadExtendedHeader(wxTarHeaderRecords*& recs)
if (!recs)
recs = new wxTarHeaderRecords;
+ // read the declared size as 64-bit before narrowing, to avoid truncation
+ const wxTarNumber declared = m_hdr->GetOctal(TAR_SIZE);
+
+ // reject an unreasonably large extended header to avoid excessive allocation
+ if (declared < 0 || declared > (wxTarNumber)GetMaxExtendedHeaderSize()) {
+ wxLogWarning(_("invalid data in extended tar header"));
+ return false;
+ }
+
+ size_t len = (size_t)declared;
Changed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Arthur Chan
@arthurscchan commented on this pull request.
> @@ -909,8 +912,26 @@ bool wxTarInputStream::ReadExtendedHeader(wxTarHeaderRecords*& recs)
if (!recs)
recs = new wxTarHeaderRecords;
+ // read the declared size as 64-bit before narrowing, to avoid truncation
+ const wxTarNumber declared = m_hdr->GetOctal(TAR_SIZE);
+
+ // reject an unreasonably large extended header to avoid excessive allocation
+ if (declared < 0 || declared > (wxTarNumber)GetMaxExtendedHeaderSize()) {
Changed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Arthur Chan
> @@ -909,8 +912,26 @@ bool wxTarInputStream::ReadExtendedHeader(wxTarHeaderRecords*& recs)
if (!recs)
recs = new wxTarHeaderRecords;
+ // read the declared size as 64-bit before narrowing, to avoid truncation
+ const wxTarNumber declared = m_hdr->GetOctal(TAR_SIZE);
+
+ // reject an unreasonably large extended header to avoid excessive allocation
+ if (declared < 0 || declared > (wxTarNumber)GetMaxExtendedHeaderSize()) {
+ wxLogWarning(_("invalid data in extended tar header"));
+ return false;
+ }
+
+ size_t len = (size_t)declared;
+
+ // don't allocate more than the stream can actually provide
True, it is more like defensive in my original though. But I do agree it is not needed. Removed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Arthur Chan
@arthurscchan commented on this pull request.
> @@ -909,8 +912,26 @@ bool wxTarInputStream::ReadExtendedHeader(wxTarHeaderRecords*& recs)
if (!recs)
recs = new wxTarHeaderRecords;
+ // read the declared size as 64-bit before narrowing, to avoid truncation
+ const wxTarNumber declared = m_hdr->GetOctal(TAR_SIZE);
+
+ // reject an unreasonably large extended header to avoid excessive allocation
+ if (declared < 0 || declared > (wxTarNumber)GetMaxExtendedHeaderSize()) {
+ wxLogWarning(_("invalid data in extended tar header"));
Fixed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.