1419278 - Uninitialised value use in lg_CopyAttribute (security/nss/lib/softoken/legacydb/lgattr.c)

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Julian Seward [:jseward]]

I saw this this morning whilst trying to reproduce bug 1418125.  I'm
not saying they are the same problem, but perhaps there is some connection?

Anyway, Valgrind complaints below.  STR:

DISPLAY=:2.0 ./mach mochitest --valgrind=vTRUNK --valgrind-args=--num-transtab-sectors=48,--px-default=allregs-at-mem-access,--px-file-backed=unwindregs-at-mem-access,--fair-sched=yes,--fullpath-after=/MOZ/,--error-limit=no,--show-mismatched-frees=no,--expensive-definedness-checks=yes,--track-origins=yes browser/components/extensions/test/browser/browser_ext_windows_create_params.js

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complaints

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

`nsslowcert_ReadDBSMimeEntry` [1] allocates `entry`, so that's just a random chunk of memory. We then try to fill that structure via `DecodeDBSMimeEntry` [2], at this point the SECItem.data fields are still uninitialized. We set .data = alloc() only when the associated .len field is > 0 [3].

That means we'll call memcpy() with a random destination, but the count being zero. I assume we could "fix" this by changing the check to (value != NULL && len > 0) in `lg_CopyAttribute` [4].

[1] https://searchfox.org/nss/source/lib/softoken/legacydb/pcertdb.c#2034
[2] https://searchfox.org/nss/source/lib/softoken/legacydb/pcertdb.c#2059
[3] https://searchfox.org/nss/source/lib/softoken/legacydb/pcertdb.c#1845
[4] https://searchfox.org/nss/source/lib/softoken/legacydb/lgattr.c#136

Comment 3

[Julian Seward [:jseward]]

Attachment: Investigation patch that confirms Tim's analysis. DO NOT LAND.

Comment 4

[Julian Seward [:jseward]]

(In reply to Tim Taubert [:ttaubert] from comment #2)

Thanks for the analysis.  Using the patch in comment 3 I can confirm that
analysis, without using valgrind.

> I assume we could "fix" this by changing the check to
> (value != NULL && len > 0) in `lg_CopyAttribute` [4].

Except we'd still be doing "<uninitialised> != NULL" like that.  If we
swap the order then it's good, yes?

  len > 0 && value != NULL

I'll try that.  Given that this is sec- related code, I'm vaguely
uncomfortable that we have uninitialised values drifting around the
computation and only "catching" them mid-computation.  Is there there
any enthusiasm for using calloc here, as a safety measure?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Julian Seward [:jseward] from comment #4)
> Except we'd still be doing "<uninitialised> != NULL" like that.  If we
> swap the order then it's good, yes?
> 
>   len > 0 && value != NULL

Good point, let's do that.

> I'll try that.  Given that this is sec- related code, I'm vaguely
> uncomfortable that we have uninitialised values drifting around the
> computation and only "catching" them mid-computation.  Is there there
> any enthusiasm for using calloc here, as a safety measure?

Yeah, I voiced similar concerns on phabricator. We could use PORT_ArenaZAlloc() instead of PORT_ArenaAlloc(), Z for zero, to achieve the same :)

Comment 6

[Julian Seward [:jseward]]

Attachment: "Fix" per analysis in comment 2

(from comment 4)
> Except we'd still be doing "<uninitialised> != NULL" like that.  If we
> swap the order then it's good, yes?
>
>  len > 0 && value != NULL

Yes, that seems to work.

Comment 7

[Phabricator Automation]

Comment on attachment 8930366 [details]
Bug 1419278 - maybe fixing something

Tim Taubert [:ttaubert] has approved the revision.

https://phabricator.services.mozilla.com/D267#6890

Comment 8

[Julian Seward [:jseward]]

(In reply to Phabricator Automation from comment #7)
> Comment on attachment 8930366 [details]
> Bug 1419278 - maybe fixing something

That fixes the V errors.  So, +1 from me.

Comment 9

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/96e6dbbd080f07b325a31e67fea8ed858c6b21cf