Address some security concerns in the cgpt tool.

Address some security concerns in the cgpt tool.

1. Check for potential integer overflow in sector_bytes * sector_count.
2. Added O_NOFOLLOW to open() call - Is this enough?
3. Passing buffer length to GuidToStr(), PMBRToStr().
4. Use unsigned int in GetEntry() to determine stride.
5. Address conversion between UTF16 and UTF8.

Note: The UTF conversion is complex and troublesome, and needs careful
consideration to get right. For now, I've just forced the interpretation of
the partition name to 7-bit ASCII. That's sufficient for the needs of Chrome
OS, and I can file a new issue to handle UTF correctly.


BUG=chrome-os-partner:705
TEST=manual

Running "make runtests" invokes the tests/run_cgpt_tests.sh script, which checks the behavior and output of the cgpt tool.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=c4e92af

[gauravsh, 2010-10-06 18:45:53]

Drive-by comments...

Also, even though this is a re-factor, doesn't mean it shouldn't be tested. The
cgpt tool is a critical part of the build process - so please add a better TEST
description with how you verified your changes.

http://codereview.chromium.org/3594010/diff/1/3
File cgpt/cgpt_common.c (right):

http://codereview.chromium.org/3594010/diff/1/3#newcode328
cgpt/cgpt_common.c:328: assert(buflen >= 37);
how do you get to this "37" number. comment?

http://codereview.chromium.org/3594010/diff/1/3#newcode329
cgpt/cgpt_common.c:329: snprintf(str, buflen,
"%08X-%04X-%04X-%02X%02X-%02X%02X%02X%02X%02X%02X",
should you check for snprintf failure?

http://codereview.chromium.org/3594010/diff/1/3#newcode341
cgpt/cgpt_common.c:341: * temporary fix, I'm making this ONLY support ASCII
codepoints. A separate
Can you just reference the filed bug here?

http://codereview.chromium.org/3594010/diff/1/3#newcode465
cgpt/cgpt_common.c:465: if (entry_index < 0 || entry_index >= max)
I think the promotion rules for signed/unsigned comparison are compiler
implementation dependent, and this may or may not work.

if (entry_index <0 || (uint32_t)entry_index >= max) might be better.

Also, a general comment - does this code always run on a 32-bit system? What if
it is being run on the chroot host? Since you are assuming that sizeof(int) ==
sizeof(uint32_t), will this overflow testing loging fail for 64-bit systems.

http://codereview.chromium.org/3594010/diff/1/5
File cgpt/cmd_boot.c (right):

http://codereview.chromium.org/3594010/diff/1/5#newcode157
cgpt/cmd_boot.c:157: GuidToStr(&drive.pmbr.boot_guid, buf, 256);
s/256/sizeof(buf)/

in case the length of the allocated buf changes in the future.

http://codereview.chromium.org/3594010/diff/1/6
File cgpt/cmd_find.c (right):

http://codereview.chromium.org/3594010/diff/1/6#newcode163
cgpt/cmd_find.c:163: char partlabel[256];
magic numbers are usually discouraged. can you make this a named constant? It is
used at many places. (MAX_LABEL_SIZE or something similar)

http://codereview.chromium.org/3594010/diff/1/6#newcode184
cgpt/cmd_find.c:184: UTF16ToUTF8(entry->name, sizeof(entry->name), (uint8_t
*)partlabel, 256);
sizeof(partlabel)

[Bill Richardson, 2010-10-08 21:56:45]

Uploading new version. Please take another look.

http://codereview.chromium.org/3594010/diff/1/3
File cgpt/cgpt_common.c (right):

http://codereview.chromium.org/3594010/diff/1/3#newcode328
cgpt/cgpt_common.c:328: assert(buflen >= 37);
On 2010/10/06 18:45:53, gauravsh wrote:
> how do you get to this "37" number. comment?

It's the length of the formatted UUID + '\0'. Comment added.

http://codereview.chromium.org/3594010/diff/1/3#newcode329
cgpt/cgpt_common.c:329: snprintf(str, buflen,
"%08X-%04X-%04X-%02X%02X-%02X%02X%02X%02X%02X%02X",
On 2010/10/06 18:45:53, gauravsh wrote:
> should you check for snprintf failure?

I guess. I can't imagine why it wouldn't work, so I'll make it an assert.

http://codereview.chromium.org/3594010/diff/1/3#newcode341
cgpt/cgpt_common.c:341: * temporary fix, I'm making this ONLY support ASCII
codepoints. A separate
On 2010/10/06 18:45:53, gauravsh wrote:
> Can you just reference the filed bug here?

Done.

http://codereview.chromium.org/3594010/diff/1/3#newcode465
cgpt/cgpt_common.c:465: if (entry_index < 0 || entry_index >= max)
No, it'll fail when sizeof(int) == sizeof(uint32_t), which is the case even on
64-bit linux. 

A signed int isn't big enough to represent all the possible indices. I'll have
to change all the places it's called from.

http://codereview.chromium.org/3594010/diff/1/5
File cgpt/cmd_boot.c (right):

http://codereview.chromium.org/3594010/diff/1/5#newcode157
cgpt/cmd_boot.c:157: GuidToStr(&drive.pmbr.boot_guid, buf, 256);
done. Also in a few other places.

http://codereview.chromium.org/3594010/diff/1/6
File cgpt/cmd_find.c (right):

http://codereview.chromium.org/3594010/diff/1/6#newcode163
cgpt/cmd_find.c:163: char partlabel[256];
Okay.

http://codereview.chromium.org/3594010/diff/1/6#newcode184
cgpt/cmd_find.c:184: UTF16ToUTF8(entry->name, sizeof(entry->name), (uint8_t
*)partlabel, 256);
done.

[gauravsh, 2010-10-09 22:59:35]

LGTM with a couple of comments about scratch buffer lengths and error checking
functions which accept the max length for an output buffer.

http://codereview.chromium.org/3594010/diff/8001/9002
File cgpt/cgpt_common.c (right):

http://codereview.chromium.org/3594010/diff/8001/9002#newcode23
cgpt/cgpt_common.c:23: #include <stdarg.h>
nit: alpha order for includes

http://codereview.chromium.org/3594010/diff/8001/9002#newcode84
cgpt/cgpt_common.c:84: if (sector_bytes > (UINT64_MAX / sector_count)) {
good catch, totally missed the inverted check in the first patch.

http://codereview.chromium.org/3594010/diff/8001/9007
File cgpt/cmd_show.c (right):

http://codereview.chromium.org/3594010/diff/8001/9007#newcode123
cgpt/cmd_show.c:123: snprintf(contents, sizeof(contents), "Label: \"%s\"",
label);
I would error check this snprintf since given the buffer lengths, it can fail
(the only reason it doesn't is because you fill |label| using UTF16toUTF8 but
this might trip us if the code changes in the future).

http://codereview.chromium.org/3594010/diff/8001/9007#newcode344
cgpt/cmd_show.c:344: char buf[256];                      // buffer for formatted
PMBR content
So you use these hard coded buffer sizes for scratch buffer. But (e.g. in this
case), is it guaranteed that the formatted contents will be max 255 bytes - if
so, how?

The buffer size gets passed around - but you don't check for errors (PMBRToStr()
below). Should you check it?

[Bill Richardson, 2010-10-11 16:26:59]

http://codereview.chromium.org/3594010/diff/8001/9007
File cgpt/cmd_show.c (right):

http://codereview.chromium.org/3594010/diff/8001/9007#newcode123
cgpt/cmd_show.c:123: snprintf(contents, sizeof(contents), "Label: \"%s\"",
label);
I'm not sure it matters, since I only use it to maintain the formatting of the
entire line, but okay. There are few other cases too.

http://codereview.chromium.org/3594010/diff/8001/9007#newcode344
cgpt/cmd_show.c:344: char buf[256];                      // buffer for formatted
PMBR content
PMBRToStr() will exit if buf is too small.

[Bill Richardson, 2010-10-11 16:27:40]

Gaurav, would you take one more look just to be sure.
Thanks.

[gauravsh, 2010-10-12 03:35:34]

Still LGTM. Thanks!

On 2010/10/11 16:27:40, Bill Richardson wrote:
> Gaurav, would you take one more look just to be sure.
> Thanks.