Adding checks (part 2/2) and minor changes

Adding checks (part 2/2) and minor changes

- Added further checks
- Removed commenting-out code
- Fixed style ("type &variable" -> "type& variable")
- In RecunstructTransformed(), the 6th argument of RecunstructGlyf()
  call should be |loca_table->dst_length|, not |loca_table->dst_offset|.

BUG=none
TEST=compiled

[bashi, 2012-05-02 05:36:34]

Hi Raph,

The second (and hopefully the last) CL to add checks. Could you take a look?

[agl, 2012-05-02 16:56:18]

LGTM. However, I am concerned that this code's journey has been such that
security was added afterwards. Since this is frontline code, I think a dedicated
security review is needed. If one isn't already scheduled, please point me at
the documentation for the format and I can try to find the time.

https://codereview.appspot.com/6160043/diff/1/cpp/woff2.cc
File cpp/woff2.cc (right):

https://codereview.appspot.com/6160043/diff/1/cpp/woff2.cc#newcode376
cpp/woff2.cc:376: if (loca_offset > glyf_buf_length - 2 - 10) {
It's not immediately clear that glyf_buf_length >= 12. Could you make it:

if (glyf_buf_length < 2 + 10 ||
    loca_offset > glyf_buf_length - 2 - 10) {
  ...
}

to make it clear that we don't underflow?

[bashi, 2012-05-07 08:31:30]

Thank you for review and sorry for late response. I haven't asked security
review so I'd really appreciate if you could take time for it. Here is the
document:
http://wiki.font-compression-reference.googlecode.com/git/img/WOFFUltraConden...

http://codereview.appspot.com/6160043/diff/1/cpp/woff2.cc
File cpp/woff2.cc (right):

http://codereview.appspot.com/6160043/diff/1/cpp/woff2.cc#newcode376
cpp/woff2.cc:376: if (loca_offset > glyf_buf_length - 2 - 10) {
On 2012/05/02 16:56:18, agl wrote:
> It's not immediately clear that glyf_buf_length >= 12. Could you make it:
> 
> if (glyf_buf_length < 2 + 10 ||
>     loca_offset > glyf_buf_length - 2 - 10) {
>   ...
> }
> 
> to make it clear that we don't underflow?

Done.

[agl, 2012-05-14 19:55:27]

On Mon, May 7, 2012 at 4:31 AM,  <bashi@chromium.org> wrote:
> Thank you for review and sorry for late response. I haven't asked
> security review so I'd really appreciate if you could take time for it.
> Here is the document:
>
http://wiki.font-compression-reference.googlecode.com/git/img/WOFFUltraConden...

I've attached notes as a patch to the file. Some hunks are suggested
changes and some are simply comments. The comments are not supposed to
end up in the final code :)

I'm afraid that I found the code difficult to review. Many of the
preconditions to make the code correct come from a long way away and
there's no explicit commenting of preconditions in the code. I'm
afraid that I'm likely to have missed bugs in this code and it would
be a significant addition to our attack surface were it to be included
in Chrome.


Cheers

AGL

[bashi, 2012-05-15 00:11:33]

Hi agl,

Thank you so much for your time to this! I'll apply your suggestions later.

I agree that the current code difficult to review. Raph, could you help me with
refactoring and adding comments when you have time?
 
On 2012/05/14 19:55:27, agl wrote:
> On Mon, May 7, 2012 at 4:31 AM,  <mailto:bashi@chromium.org> wrote:
> > Thank you for review and sorry for late response. I haven't asked
> > security review so I'd really appreciate if you could take time for it.
> > Here is the document:
> >
>
http://wiki.font-compression-reference.googlecode.com/git/img/WOFFUltraConden...
> 
> I've attached notes as a patch to the file. Some hunks are suggested
> changes and some are simply comments. The comments are not supposed to
> end up in the final code :)
> 
> I'm afraid that I found the code difficult to review. Many of the
> preconditions to make the code correct come from a long way away and
> there's no explicit commenting of preconditions in the code. I'm
> afraid that I'm likely to have missed bugs in this code and it would
> be a significant addition to our attack surface were it to be included
> in Chrome.
> 
> 
> Cheers
> 
> AGL

[raph, 2012-05-15 00:16:14]

Yes, I am happy to help make the code simpler to understand, I think that's
a good thing overall. I should be able to give it some time tomorrow.

Thanks,

Raph

On Mon, May 14, 2012 at 5:11 PM, <bashi@chromium.org> wrote:

> Hi agl,
>
> Thank you so much for your time to this! I'll apply your suggestions
> later.
>
> I agree that the current code difficult to review. Raph, could you help
> me with refactoring and adding comments when you have time?
>
> On 2012/05/14 19:55:27, agl wrote:
>
>  On Mon, May 7, 2012 at 4:31 AM,  <mailto:bashi@chromium.org> wrote:
>> > Thank you for review and sorry for late response. I haven't asked
>> > security review so I'd really appreciate if you could take time for
>>
> it.
>
>> > Here is the document:
>> >
>>
>
> http://wiki.font-compression-**reference.googlecode.com/git/**img/**
>
WOFFUltraCondensedfileformat.**pdf<http://wiki.font-compression-reference.googlecode.com/git/img/WOFFUltraCondensedfileformat.pdf>
>
>  I've attached notes as a patch to the file. Some hunks are suggested
>> changes and some are simply comments. The comments are not supposed to
>> end up in the final code :)
>>
>
>  I'm afraid that I found the code difficult to review. Many of the
>> preconditions to make the code correct come from a long way away and
>> there's no explicit commenting of preconditions in the code. I'm
>> afraid that I'm likely to have missed bugs in this code and it would
>> be a significant addition to our attack surface were it to be included
>> in Chrome.
>>
>
>
>  Cheers
>>
>
>  AGL
>>
>
>
>
>
http://codereview.appspot.com/**6160043/<http://codereview.appspot.com/6160043/>
>

[bashi, 2012-05-15 09:54:04]

I didn't have enough time for this today so I just merged agl's suggestion to
the CL. Raph, please use the latest CL if you want to make change tomorrow.

Thanks,

On 2012/05/15 00:16:14, raph wrote:
> Yes, I am happy to help make the code simpler to understand, I think that's
> a good thing overall. I should be able to give it some time tomorrow.
> 
> Thanks,
> 
> Raph
> 
> On Mon, May 14, 2012 at 5:11 PM, <mailto:bashi@chromium.org> wrote:
> 
> > Hi agl,
> >
> > Thank you so much for your time to this! I'll apply your suggestions
> > later.
> >
> > I agree that the current code difficult to review. Raph, could you help
> > me with refactoring and adding comments when you have time?
> >
> > On 2012/05/14 19:55:27, agl wrote:
> >
> >  On Mon, May 7, 2012 at 4:31 AM,  <mailto:bashi@chromium.org> wrote:
> >> > Thank you for review and sorry for late response. I haven't asked
> >> > security review so I'd really appreciate if you could take time for
> >>
> > it.
> >
> >> > Here is the document:
> >> >
> >>
> >
> > http://wiki.font-compression-**reference.googlecode.com/git/**img/**
> >
>
WOFFUltraCondensedfileformat.**pdf<http://wiki.font-compression-reference.googlecode.com/git/img/WOFFUltraCondensedfileformat.pdf>
> >
> >  I've attached notes as a patch to the file. Some hunks are suggested
> >> changes and some are simply comments. The comments are not supposed to
> >> end up in the final code :)
> >>
> >
> >  I'm afraid that I found the code difficult to review. Many of the
> >> preconditions to make the code correct come from a long way away and
> >> there's no explicit commenting of preconditions in the code. I'm
> >> afraid that I'm likely to have missed bugs in this code and it would
> >> be a significant addition to our attack surface were it to be included
> >> in Chrome.
> >>
> >
> >
> >  Cheers
> >>
> >
> >  AGL
> >>
> >
> >
> >
> >
>
http://codereview.appspot.com/**6160043/%3Chttp://codereview.appspot.com/6160...>
> >

[raph, 2012-05-18 01:07:40]

A few really good catches in here, and bunch of other places where additional
tests are probably technically redundant, but improve the confidence in the code
and robustness to future changes.

I'm sorry the code was difficult to review. It's fairly dense in that it's doing
quite a bit. I'm preparing a followup CL (based on this one, so I think this one
can be committed as is, and I will fix the places highlighted that need further
changes) that adds quite a bit of commenting.

I'm open to ideas of what's the best way to make it even better. Obviously more
comments. Would comprehensive unit tests help? And I think one of the ways to
get more confidence in security would be thorough fuzz testing. I've been
intending to prepare these, and feel they would be very helpful for other
independent implementations.

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc
File cpp/woff2.cc (right):

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode73
cpp/woff2.cc:73: // agl: This doesn't seem to match the value in the spec, which
is 16.
The value in _long_ form is 16, the value in _short_ form is 3. I think long
form is going away, so I think the code is okay, but the spec needs to be edited
to be short form cleanly only.

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode259
cpp/woff2.cc:259: // comment and/or an assert would be good.
Yes, n_contours is a 16 bit value. Best to assert, I think.

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode385
cpp/woff2.cc:385: // comment and/or assert is needed to document that.
Agree.

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode394
cpp/woff2.cc:394: // has n_glyphs+1 elements should be documented.
Agree.

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode494
cpp/woff2.cc:494: // initially.
It seems to me that if this condition is false, one of the Read operations will
fail (it reads in all bytes up to the header), but I have no objections to
redundant checks like this, as they'll be far more robust to code changes.

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode591
cpp/woff2.cc:591: // agl: this fails if end_point > 65535, does that matter?
It should be checked and fail on overflow.

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode662
cpp/woff2.cc:662: if (static_cast<uint64_t>(glyf_table->dst_offset +
glyf_table->dst_length) >
Again, I think these tests are redundant because, after the header is parsed,
all Table objects will meet the invariant that offset + dst_length <=
result_length, but checking them again is harmless.

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode672
cpp/woff2.cc:672: dst + loca_table->dst_offset, loca_table->dst_length);
Good catch.

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode691
cpp/woff2.cc:691: // that addition is mod 2^32 is well defined.
Yes, thanks. I got confused by the fact that signed overflow is undefined
behavior.

https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode729
cpp/woff2.cc:729: // agl: I think src_size should be dst_size in the next line.
Yes. Obviously the gzip paths need to be better tested, sorry about that.

[bashi, 2012-06-04 05:34:01]

Sorry for late response. I was in a hospital last two weeks.

I committed the CL as-is so that Raph can address agl's comments.

On 2012/05/18 01:07:40, raph wrote:
> A few really good catches in here, and bunch of other places where additional
> tests are probably technically redundant, but improve the confidence in the
code
> and robustness to future changes.
> 
> I'm sorry the code was difficult to review. It's fairly dense in that it's
doing
> quite a bit. I'm preparing a followup CL (based on this one, so I think this
one
> can be committed as is, and I will fix the places highlighted that need
further
> changes) that adds quite a bit of commenting.
> 
> I'm open to ideas of what's the best way to make it even better. Obviously
more
> comments. Would comprehensive unit tests help? And I think one of the ways to
> get more confidence in security would be thorough fuzz testing. I've been
> intending to prepare these, and feel they would be very helpful for other
> independent implementations.
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc
> File cpp/woff2.cc (right):
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode73
> cpp/woff2.cc:73: // agl: This doesn't seem to match the value in the spec,
which
> is 16.
> The value in _long_ form is 16, the value in _short_ form is 3. I think long
> form is going away, so I think the code is okay, but the spec needs to be
edited
> to be short form cleanly only.
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode259
> cpp/woff2.cc:259: // comment and/or an assert would be good.
> Yes, n_contours is a 16 bit value. Best to assert, I think.
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode385
> cpp/woff2.cc:385: // comment and/or assert is needed to document that.
> Agree.
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode394
> cpp/woff2.cc:394: // has n_glyphs+1 elements should be documented.
> Agree.
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode494
> cpp/woff2.cc:494: // initially.
> It seems to me that if this condition is false, one of the Read operations
will
> fail (it reads in all bytes up to the header), but I have no objections to
> redundant checks like this, as they'll be far more robust to code changes.
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode591
> cpp/woff2.cc:591: // agl: this fails if end_point > 65535, does that matter?
> It should be checked and fail on overflow.
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode662
> cpp/woff2.cc:662: if (static_cast<uint64_t>(glyf_table->dst_offset +
> glyf_table->dst_length) >
> Again, I think these tests are redundant because, after the header is parsed,
> all Table objects will meet the invariant that offset + dst_length <=
> result_length, but checking them again is harmless.
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode672
> cpp/woff2.cc:672: dst + loca_table->dst_offset, loca_table->dst_length);
> Good catch.
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode691
> cpp/woff2.cc:691: // that addition is mod 2^32 is well defined.
> Yes, thanks. I got confused by the fact that signed overflow is undefined
> behavior.
> 
> https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode729
> cpp/woff2.cc:729: // agl: I think src_size should be dst_size in the next
line.
> Yes. Obviously the gzip paths need to be better tested, sorry about that.

[raph, 2012-06-04 23:16:33]

Thanks for committing, bashi. I've been very busy with Android stuff, but
will try to get a followon CL prepared soon.

I hope the hospital visit wasn't for anything too serious and that you're
okay now.

Take care,

Raph

On Sun, Jun 3, 2012 at 10:34 PM, <bashi@chromium.org> wrote:

> Sorry for late response. I was in a hospital last two weeks.
>
> I committed the CL as-is so that Raph can address agl's comments.
>
> On 2012/05/18 01:07:40, raph wrote:
>
>> A few really good catches in here, and bunch of other places where
>>
> additional
>
>> tests are probably technically redundant, but improve the confidence
>>
> in the code
>
>> and robustness to future changes.
>>
>
>  I'm sorry the code was difficult to review. It's fairly dense in that
>>
> it's doing
>
>> quite a bit. I'm preparing a followup CL (based on this one, so I
>>
> think this one
>
>> can be committed as is, and I will fix the places highlighted that
>>
> need further
>
>> changes) that adds quite a bit of commenting.
>>
>
>  I'm open to ideas of what's the best way to make it even better.
>>
> Obviously more
>
>> comments. Would comprehensive unit tests help? And I think one of the
>>
> ways to
>
>> get more confidence in security would be thorough fuzz testing. I've
>>
> been
>
>> intending to prepare these, and feel they would be very helpful for
>>
> other
>
>> independent implementations.
>>
>
> 
https://codereview.appspot.**com/6160043/diff/11001/cpp/**woff2.cc<https://co...
>> File cpp/woff2.cc (right):
>>
>
>
> https://codereview.appspot.**com/6160043/diff/11001/cpp/**
>
woff2.cc#newcode73<https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode73>
>
>> cpp/woff2.cc:73: // agl: This doesn't seem to match the value in the
>>
> spec, which
>
>> is 16.
>> The value in _long_ form is 16, the value in _short_ form is 3. I
>>
> think long
>
>> form is going away, so I think the code is okay, but the spec needs to
>>
> be edited
>
>> to be short form cleanly only.
>>
>
>
> https://codereview.appspot.**com/6160043/diff/11001/cpp/**
>
woff2.cc#newcode259<https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode259>
>
>> cpp/woff2.cc:259: // comment and/or an assert would be good.
>> Yes, n_contours is a 16 bit value. Best to assert, I think.
>>
>
>
> https://codereview.appspot.**com/6160043/diff/11001/cpp/**
>
woff2.cc#newcode385<https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode385>
>
>> cpp/woff2.cc:385: // comment and/or assert is needed to document that.
>> Agree.
>>
>
>
> https://codereview.appspot.**com/6160043/diff/11001/cpp/**
>
woff2.cc#newcode394<https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode394>
>
>> cpp/woff2.cc:394: // has n_glyphs+1 elements should be documented.
>> Agree.
>>
>
>
> https://codereview.appspot.**com/6160043/diff/11001/cpp/**
>
woff2.cc#newcode494<https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode494>
>
>> cpp/woff2.cc:494: // initially.
>> It seems to me that if this condition is false, one of the Read
>>
> operations will
>
>> fail (it reads in all bytes up to the header), but I have no
>>
> objections to
>
>> redundant checks like this, as they'll be far more robust to code
>>
> changes.
>
>
> https://codereview.appspot.**com/6160043/diff/11001/cpp/**
>
woff2.cc#newcode591<https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode591>
>
>> cpp/woff2.cc:591: // agl: this fails if end_point > 65535, does that
>>
> matter?
>
>> It should be checked and fail on overflow.
>>
>
>
> https://codereview.appspot.**com/6160043/diff/11001/cpp/**
>
woff2.cc#newcode662<https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode662>
>
>> cpp/woff2.cc:662: if (static_cast<uint64_t>(glyf_**table->dst_offset +
>> glyf_table->dst_length) >
>> Again, I think these tests are redundant because, after the header is
>>
> parsed,
>
>> all Table objects will meet the invariant that offset + dst_length <=
>> result_length, but checking them again is harmless.
>>
>
>
> https://codereview.appspot.**com/6160043/diff/11001/cpp/**
>
woff2.cc#newcode672<https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode672>
>
>> cpp/woff2.cc:672: dst + loca_table->dst_offset,
>>
> loca_table->dst_length);
>
>> Good catch.
>>
>
>
> https://codereview.appspot.**com/6160043/diff/11001/cpp/**
>
woff2.cc#newcode691<https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode691>
>
>> cpp/woff2.cc:691: // that addition is mod 2^32 is well defined.
>> Yes, thanks. I got confused by the fact that signed overflow is
>>
> undefined
>
>> behavior.
>>
>
>
> https://codereview.appspot.**com/6160043/diff/11001/cpp/**
>
woff2.cc#newcode729<https://codereview.appspot.com/6160043/diff/11001/cpp/woff2.cc#newcode729>
>
>> cpp/woff2.cc:729: // agl: I think src_size should be dst_size in the
>>
> next line.
>
>> Yes. Obviously the gzip paths need to be better tested, sorry about
>>
> that.
>
>
>
>
http://codereview.appspot.com/**6160043/<http://codereview.appspot.com/6160043/>
>