Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
322333002:
Fix the potential integer overflow from "offset + size" (Closed)
|
Created:
6 years, 6 months ago by jun_fang Modified:
6 years, 5 months ago CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@master Visibility:
Public. |
DescriptionThis change is for fixing the potential integer overflow from "offset + size"
BUG=382667
R=palmer@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/3414a64
Patch Set 1 #
Total comments: 3
Patch Set 2 : check integer overflow with numeric #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 9
Patch Set 5 : #Patch Set 6 : some formats in the fix of offest+size #
Messages
Total messages: 21 (0 generated)
You are invited to review code change for fixing the potential integer overflow from "offset + size". Please let me know if you have any comments.
https://codereview.chromium.org/322333002/diff/1/core/src/fpdfapi/fpdf_parser... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/322333002/diff/1/core/src/fpdfapi/fpdf_parser... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2858: if(size <= 0 || offset < 0 || offset > m_dwFileLen) According to ./core/include/fxcrt/fx_system.h:110:typedef FX_DWORD FX_UINT32; |size| can never be < 0, so "<=" is "testing too much", if you see what I mean. Style nit: Other code in this file uses a space after the "if": "if (...)". https://codereview.chromium.org/322333002/diff/1/core/src/fpdfapi/fpdf_parser... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2861: size = (FX_DWORD)(offset + size + 512 > m_dwFileLen ? m_dwFileLen - offset : size + 512); The expression "offset + size + 512" can overflow, which might change the outcome of this calculation in a way you don't intend. (I.e. you might get "size + 512" when you wanted "m_dwFileLen - offset". The annoyance of writing 2 checks for the 2 additions, plus the check for size + 512, is a good argument for using our base/numerics library from Chromium. I think the explicit cast to FX_DWORD might not be necessary? It's what happens implicitly anyway. Also, there is a logical problem with "FX_FILESIZE offset". On some platforms, FX_FILESIZE is off_t, which is sometimes 64-bit. First, there is a problem with the idea of having a file that might be very large (> 4 GiB) but having its size be described with the FX_UINT32/FX_DWORD variable |size|. Second, in various places in the code, you cast/truncated |m_dwFileLen| to a FX_DWORD anyway. So, you should decide if you want to support large files, and if so, consistently use a 64-bit value to describe the size of the file and the objects in it (it might be possible to have individual objects in the file be > 4 GiB, like an embedded video...?). If you don't want to support large files, it's probably better to fail early when parsing them rather than truncating their size to 4 GiB, which is effectively what is happening now. Finally, note that if offset == m_dwFileLen, you can end up with size = 0. Which you don't want (you check for it on line 2858).
Any update on this patch?
https://codereview.chromium.org/322333002/diff/1/core/src/fpdfapi/fpdf_parser... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/322333002/diff/1/core/src/fpdfapi/fpdf_parser... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2858: if(size <= 0 || offset < 0 || offset > m_dwFileLen) On 2014/06/12 00:32:05, Chromium Palmer wrote: > According to > > ./core/include/fxcrt/fx_system.h:110:typedef FX_DWORD > FX_UINT32; > > |size| can never be < 0, so "<=" is "testing too much", if you see what I mean. > > Style nit: Other code in this file uses a space after the "if": "if (...)". You are right. I will change "<=" to "==" and change the style back based on the your comments.
https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2872: base::CheckedNumeric<FX_DWORD> safe_size = size; You should check the value that you actually use, i.e. |size|. You don't need |safe_size|. base::CheckedNumeric<FX_DWORD> size = GetObjectSize(pRef->GetRefObjNum(), offset); if (size.ValueOrDefault(0) == 0 || offset < 0 || offset >= m_dwFileLen) { break; } safe_size += offset; safe_size += 512; if (!safe_size.IsValid()) { break; } if (safe_size.ValueOrDie() > m_dwFileLen) { size = m_dwFileLen - offset; } else { size = size + 512; } if (!safe_size.IsValid()) { break; } Then use size.ValidOrDie() on lines 2883 and 2884. https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3089: base::CheckedNumeric<FX_DWORD> safe_size = size; Same as above. https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3115: base::CheckedNumeric<FX_DWORD> safe_size = size; Same as above.
> safe_size += offset;
> safe_size += 512;
> if (!safe_size.IsValid()) {
> break;
> }
>
> if (safe_size.ValueOrDie() > m_dwFileLen) {
> size = m_dwFileLen - offset;
> } else {
> size = size + 512;
> }
> if (!safe_size.IsValid()) {
> break;
> }
Oops, sorry; all those |safe_size|s should be |size|.
https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2872: base::CheckedNumeric<FX_DWORD> safe_size = size; On 2014/07/09 18:29:14, Chromium Palmer wrote: > You should check the value that you actually use, i.e. |size|. You don't need > |safe_size|. > > base::CheckedNumeric<FX_DWORD> size = GetObjectSize(pRef->GetRefObjNum(), > offset); > if (size.ValueOrDefault(0) == 0 || offset < 0 || offset >= m_dwFileLen) { > break; > } > > safe_size += offset; > safe_size += 512; > if (!safe_size.IsValid()) { > break; > } > > if (safe_size.ValueOrDie() > m_dwFileLen) { > size = m_dwFileLen - offset; > } else { > size = size + 512; > } > if (!safe_size.IsValid()) { > break; > } > > Then use size.ValidOrDie() on lines 2883 and 2884. safe_size is a temporary variable which is used to judge whether the result of |size+offset+512| overflows. Even we use the method you suggested: base::CheckedNumeric<FX_DWORD> size = GetObjectSize(pRef->GetRefObjNum(). We still need another temporary variable to check the sum of integer. https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2879: size = m_dwFileLen - offset; size = m_dwFileLen - offset; |size| should be valid because we check |offset < m_dwFileLen| at the line of 2869. https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2881: size = size + 512; because safe_size is valid, we can make sure size is valid here. safe_size = size + offset + 512;
> safe_size is a temporary variable which is used to judge whether the result of > |size+offset+512| overflows. Even we use the method you suggested: > base::CheckedNumeric<FX_DWORD> size = GetObjectSize(pRef->GetRefObjNum(). We > still need another temporary variable to check the sum of integer. In my testing, once a CheckedNumeric has overflowed, it stays invalid even if subsequent additions operations on it do not overflow. So you should be able to use a single CheckedNumeric, do with it what you need, and then use it with ValueOrDefault or ValueOrDie. > https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2879: size = m_dwFileLen - > offset; > size = m_dwFileLen - offset; |size| should be valid because we check |offset < > m_dwFileLen| at the line of 2869. > > https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2881: size = size + 512; > because safe_size is valid, we can make sure size is valid here. > safe_size = size + offset + 512; Perhaps, but why not just use the known-safe CheckedNumeric for all checking?
+jschuh to make sure I am on the right track.
On 2014/07/10 22:47:16, Chromium Palmer wrote: > > safe_size is a temporary variable which is used to judge whether the result of > > |size+offset+512| overflows. Even we use the method you suggested: > > base::CheckedNumeric<FX_DWORD> size = GetObjectSize(pRef->GetRefObjNum(). We > > still need another temporary variable to check the sum of integer. > > In my testing, once a CheckedNumeric has overflowed, it stays invalid even if > subsequent additions operations on it do not overflow. So you should be able to > use a single CheckedNumeric, do with it what you need, and then use it with > ValueOrDefault or ValueOrDie. > size will be used after this checking. So I need to add a temporary safe_size to check whether |size+offset+512| overflows. You are right. Once a CheckedNumeric has overflowed, it stays invalid. So we don't check the result of each operation. For example, we only need to check the result of size+offset+512 rather than both size+offset and size+offset+512. > > > https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... > > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2879: size = m_dwFileLen - > > offset; > > size = m_dwFileLen - offset; |size| should be valid because we check |offset < > > m_dwFileLen| at the line of 2869. > > > > > https://codereview.chromium.org/322333002/diff/40001/core/src/fpdfapi/fpdf_pa... > > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2881: size = size + 512; > > because safe_size is valid, we can make sure size is valid here. > > safe_size = size + offset + 512; > > Perhaps, but why not just use the known-safe CheckedNumeric for all checking? It will make code not clear when we add the CheckedNumeric for all checking. I think we don't need to check somewhere if we make sure the result must be valid.
See https://codereview.chromium.org/390983007/ for my approach.
On 2014/07/15 01:52:24, Chromium Palmer wrote: > See https://codereview.chromium.org/390983007/ for my approach. Yep, that is the correct way to use the CheckedNumeric.
On 2014/07/18 23:14:46, jun_fang wrote: ping Chris. This fix covers more "offset + size". Can you take a look?
https://codereview.chromium.org/322333002/diff/60001/core/src/fpdfapi/fpdf_pa... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/322333002/diff/60001/core/src/fpdfapi/fpdf_pa... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2868: base::CheckedNumeric<FX_DWORD> size = original_size; I've already checked in this change (https://codereview.chromium.org/390983007/). https://codereview.chromium.org/322333002/diff/60001/core/src/fpdfapi/fpdf_pa... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3118: base::CheckedNumeric<FX_DWORD> size = original_size; So, this same basic chunk is now repeated 3 times. It's time to separate it out into its own utility function, if that's possible. https://codereview.chromium.org/322333002/diff/60001/core/src/fxcrt/extension.h File core/src/fxcrt/extension.h (right): https://codereview.chromium.org/322333002/diff/60001/core/src/fxcrt/extension... core/src/fxcrt/extension.h:188: range += size; BUG: The original code checked offset + size, but this code checks size + size. https://codereview.chromium.org/322333002/diff/60001/core/src/fxcrt/extension... core/src/fxcrt/extension.h:221: if (newPos.ValueOrDefault(0) == 0 || newPos.ValueOrDie() > m_nCurSize) { ValueOrDie will cause a program abort, and you'll never get to return FALSE. FWIW. If you want to make sure to return FALSE and not die on integer overflow, try this: base::CheckedNumeric<size_t> newPos = size; newPos += offset; if (!newPos.IsValid() || newPos.ValueOrDefault(0) == 0 || newPos.ValueOrDie() > m_nCurSize) { return FALSE; } It looks a bit weird, but it's OK because !newPos.IsValid() is evaluated first — newPos.ValueOrDie() will either not die, or not get called at all.
https://codereview.chromium.org/322333002/diff/60001/core/src/fpdfapi/fpdf_pa... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/322333002/diff/60001/core/src/fpdfapi/fpdf_pa... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2868: base::CheckedNumeric<FX_DWORD> size = original_size; On 2014/07/23 17:58:36, Chromium Palmer wrote: > I've already checked in this change > (https://codereview.chromium.org/390983007/). Yes. I already merged your change into this fix by resolving the conflicting. https://codereview.chromium.org/322333002/diff/60001/core/src/fpdfapi/fpdf_pa... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3118: base::CheckedNumeric<FX_DWORD> size = original_size; On 2014/07/23 17:58:36, Chromium Palmer wrote: > So, this same basic chunk is now repeated 3 times. It's time to separate it out > into its own utility function, if that's possible. I will reconstruct this function to remove the redundant parts. https://codereview.chromium.org/322333002/diff/60001/core/src/fxcrt/extension.h File core/src/fxcrt/extension.h (right): https://codereview.chromium.org/322333002/diff/60001/core/src/fxcrt/extension... core/src/fxcrt/extension.h:188: range += size; On 2014/07/23 17:58:36, Chromium Palmer wrote: > BUG: The original code checked offset + size, but this code checks size + size. This issue was reported in the original bug. https://codereview.chromium.org/322333002/diff/60001/core/src/fxcrt/extension... core/src/fxcrt/extension.h:221: if (newPos.ValueOrDefault(0) == 0 || newPos.ValueOrDie() > m_nCurSize) { On 2014/07/23 17:58:36, Chromium Palmer wrote: > ValueOrDie will cause a program abort, and you'll never get to return FALSE. > FWIW. If you want to make sure to return FALSE and not die on integer overflow, > try this: > > base::CheckedNumeric<size_t> newPos = size; > newPos += offset; > if (!newPos.IsValid() || newPos.ValueOrDefault(0) == 0 || newPos.ValueOrDie() > > m_nCurSize) { > return FALSE; > } > > It looks a bit weird, but it's OK because !newPos.IsValid() is evaluated first — > newPos.ValueOrDie() will either not die, or not get called at all. Will update.
https://codereview.chromium.org/322333002/diff/60001/core/src/fxcrt/extension.h File core/src/fxcrt/extension.h (right): https://codereview.chromium.org/322333002/diff/60001/core/src/fxcrt/extension... core/src/fxcrt/extension.h:221: if (newPos.ValueOrDefault(0) == 0 || newPos.ValueOrDie() > m_nCurSize) { On 2014/07/23 17:58:36, Chromium Palmer wrote: > ValueOrDie will cause a program abort, and you'll never get to return FALSE. > FWIW. If you want to make sure to return FALSE and not die on integer overflow, > try this: > > base::CheckedNumeric<size_t> newPos = size; > newPos += offset; > if (!newPos.IsValid() || newPos.ValueOrDefault(0) == 0 || newPos.ValueOrDie() > > m_nCurSize) { > return FALSE; > } You can skip the !newPos.IsValid() check, because it's made redundant by the newPos.ValueOrDefault(0) == 0 condition, which will short-circuit to have you return FALSE on an invalid value. > It looks a bit weird, but it's OK because !newPos.IsValid() is evaluated first — > newPos.ValueOrDie() will either not die, or not get called at all.
Hi Chris, please review it. Thanks!
lgtm
Message was sent while issue was closed.
Committed patchset #6 manually as r3414a64 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
