Opened 3 months ago

Closed 2 months ago

#30361 closed defect (fixed)

CID 1444908: MISSING_LOCK / CID 1444769: TAINTED_SCALAR

Reported by: asn Owned by: rl1987
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: coverity, regression?, 041-should
Cc: Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description

Got two new coverity issues:

*** CID 1444908:  Concurrent data access violations  (MISSING_LOCK)
/src/test/rng_test_helpers.c: 190 in testing_enable_prefilled_rng()
184     {
185       tor_assert(buflen > 0);
186       rng_mutex = tor_mutex_new();
187     
188       prefilled_rng_buffer = tor_memdup(buffer, buflen);
189       prefilled_rng_buflen = buflen;
>>>     CID 1444908:  Concurrent data access violations  (MISSING_LOCK)
>>>     Accessing "prefilled_rng_idx" without holding lock "tor_mutex_t.mutex". Elsewhere, "prefilled_rng_idx" is accessed with
>>> "tor_mutex_t.mutex" held 3 out of 4 times (1 of these accesses strongly imply that it is necessary).
190       prefilled_rng_idx = 0;
191     
192       MOCK(crypto_rand, crypto_rand_prefilled);
193       MOCK(crypto_strongest_rand_, mock_crypto_strongest_rand);
194     }
195     

** CID 1444769:  Insecure data handling  (TAINTED_SCALAR)

________________________________________________________________________________________________________
*** CID 1444769:  Insecure data handling  (TAINTED_SCALAR)
/src/feature/nodelist/microdesc.c: 540 in microdesc_cache_reload()
534       }
535     
536       journal_content = read_file_to_str(cache->journal_fname,
537                                          RFTS_IGNORE_MISSING, &st);
538       if (journal_content) {
539         cache->journal_len = (size_t) st.st_size;
>>>     CID 1444769:  Insecure data handling  (TAINTED_SCALAR)
>>>     Passing tainted variable "journal_content" to a tainted sink.
540         warn_if_nul_found(journal_content, cache->journal_len, 0,
541                           "reading microdesc journal");
542         added = microdescs_add_to_cache(cache, journal_content,
543                                         journal_content+st.st_size,
544                                         SAVED_IN_JOURNAL, 0, -1, NULL);
545         if (added) {
}}}}

Child Tickets

Change History (9)

comment:1 Changed 2 months ago by rl1987

Status: newneeds_review

comment:2 Changed 2 months ago by dgoulet

Reviewer: ahf

comment:3 Changed 2 months ago by ahf

Status: needs_reviewneeds_revision

The fix for 1444908 seems OK, but I don't think the fix for 1444769 is right. Changing the NUL bytes to ' ' seems like it will just yield weird results later iff that path is taken. We can't set the file length to the length of the string up until the first NUL byte/EOF?

comment:4 Changed 2 months ago by nickm

Keywords: regression? added

comment:5 Changed 2 months ago by nickm

Keywords: 041-should added

comment:6 Changed 2 months ago by rl1987

Owner: set to rl1987
Status: needs_revisionassigned

comment:7 Changed 2 months ago by rl1987

Status: assignedneeds_review

That makes the patch simpler, so let's try it.

https://github.com/torproject/tor/pull/1035

comment:8 Changed 2 months ago by ahf

Status: needs_reviewmerge_ready

I think this looks good.

comment:9 Changed 2 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Okay -- squashed and merged.

Note: See TracTickets for help on using tickets.