Closed Bug 472014 Opened 12 years ago Closed 11 years ago

the smartcardmonitoringthread code uses the wrong allocator and isn't careful about buffer sizes

Categories

(Core Graveyard :: Security: UI, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: timeless, Assigned: KaiE)

Details

Attachments

(1 file, 1 obsolete file)

i found this while i was looking at bug 456409 but nelson's right, they aren't related changes.
Attachment #355259 - Flags: review?(kaie)
Adding Bob to CC, FYI, as he probably wrote most of this code.
Comment on attachment 355259 [details] [diff] [review]
carefully count lengths and use the correct allocator

In your bug description and patch, you never explain WHY you need to be careful about buffer sizes.
WHY is there a bug and WHERE?


>+  int len = strlen(tokenName) + 1;
>+  PRUint32 size = len + len+sizeof(PRUint32);

Why do you use the size of len *twice* ?
I believe you only want it once.

If you want it twice, then please add a comment explaning why.


>+  // size exceeds capacity
>+  if (len > size)
>+    return;

I don't understand why you need this check.

"len" is always >= 1.
"size" = "positive" + "positive"

Are you worried that strlen(tokenName) could be that big that you get an integer overflow?

If you are, you could an unsigned for variable "len".


>+  /* this must match the allocator used in
>+   * PLHashAllocOps.freeEntry DefaultFreeEntry */
>+  char *entry = (char *)PR_Malloc(size);

This is the only change that you provide justification for, fixing the allocator.


>+  if (!entry)
>+    return;
>+
>+  memcpy(entry,&series,sizeof(PRUint32));
>+  memcpy(&entry[sizeof(PRUint32)],tokenName,len);


In total, you copy (sizeof(PRUint32)+len) bytes into the buffer.
I still don't understand why you allocated (sizeof(PRUint32)+2*len)

I know you just moved it, but I find the expression
  &entry[sizeof(PRUint32)]
difficult to read. Why don't we simply use
  entry + sizeof(PRUint32)
?
Attachment #355259 - Flags: review?(kaie) → review-
Attached patch Patch v2Splinter Review
I saw two benefits in your patch:
- fix the allocator
- really restrict the call to HashTableRemove to tokenName==0

Here is a reduced patch
Attachment #402381 - Flags: review?(rrelyea)
(In reply to comment #3)
> - really restrict the call to HashTableRemove to tokenName==0

Actually, I'm not certain about this.

Maybe the intention of the existing code is:
  "If out of memory, and no new entry can be produced, 
   then remove any existing entry associated to the slot"
?

If yes, it should be made be more explicit with a comment.
I think the only necessary change to timeless's patch is that size should be set to len +sizeof(PRUint32).

There are  3 changes I see in this patch:

1) changing up the if statement so exception conditions occur first, reducing the level of nesting of the if's . In general, that's a good change if it fits prevailing style.

2) Change of allocator. Since PL_HashTables are adopting the memory, We should use it's preferred allocator, assuming we haven't changed the allocators when we created the the table (and we haven't -- I just checked), then we should use the allocator expected by PL_HashTables. (In practice the map to the same on all platforms, but they could in theory be split). This is a good change.

3) Checking for overflow in the lengths. Timeless's description is unfortunate, because it leads to confusion. Of course the code is carefully making sure the allocated buffers are the proper length. It does not, however check for overflow. If someone presented a tokenname string of length of less than 4G then funny overflow conditions could occur in theory.

If we return a length of just less than 4G (1-3 bytes less to be exact), then adding sizeof(PRUint32) will push the overall size over 32 bit, and len becomes 1,2, or 3 -- definitely not enough space to handle such a massive copy. Memcopy may or may not pick this up depending on whether or not it treats len as signed or unsigned). It doesn't matter, though, because the PRUint32 will already have trashed several bytes. Note: this can only happen on platforms that have a larger than 32-bit address space, but still carries 32-bit lengths (4G would cover the entire address space: including mozilla itself;). Since we are using int for length, instead of size_t, this scenario is possible on a 64-bit platform (some 64-bit platforms have 32-bit ints).

This is all completely irrelevant here, however, because we know that tokenName is no more than 33 bytes long. TokenName comes from NSS, which get's it from the token, which passes it up as a fixed length space padded array of 32 bytes.

So the only purpose of this patch is to make global searches for this kind of possible error pass on by this code. We could do this with a comment, or with a proper test. Forcing all the lengths to 32bits and mixing unsigned with signed values in the compare doesn't seem correct to me. We should probably make everything a size_t, do the sanity compare, and add a comment that it is not really needed, but here to keep automatic bug search processes happy.

Anyway the test as written is definitely wrong, and should be reworked.

bob
>  - really restrict the call to HashTableRemove to tokenName==0
> 
> Actually, I'm not certain about this.

It's not a change in semantics, it's simply making the current semantics clearer.
Oh wait, you are right Kai.

That is a good question, should we remove and existing token if we get an insertion with no memory.

I think that is just an accident, but I'd have to look at the code more carefully.
OK, that condition wasn't explicitly called out, but it can happen.... If a card is pulled and a new one inserted rapidly, we may not get our event until the insert has been completed. In that case we generate a card removal to the application, then set the token and generate a card insertion. We don't clear the old token name. If we couldn't allocate, we should probably clear the old name as irrelevant.

bob
Looking at the code, that memory alloc failure, should probably be handled by skipping both events. We probably should return an error from setTokenName and not actually send the event to the app. Otherwise the app will get an insertion, but never a removal event for that token.

That is a slightly bigger patch and an altogether different issue.

bob
Attachment #402381 - Flags: review?(rrelyea) → review+
Comment on attachment 402381 [details] [diff] [review]
Patch v2

r+ rrelyea
Assignee: timeless → kaie
Keywords: checkin-needed
Attachment #355259 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/7dc49c3b5130
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.3a1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.