Opened 3 years ago

Closed 3 years ago

#19357 closed defect (wontfix)

keypin_load_journal_impl() might break if journal file contains NUL

Reported by: andrea Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: isaremoved, tor-03-unspecified-201612
Cc: Actual Points:
Parent ID: Points: .1
Reviewer: Sponsor:

Description

The journal file reader loop in src/or/keypin.c only uses end of file or '\n' to find the end of a line, so if a line contains a NUL we may end up passing a string with one in the middle to other things:

367 STATIC int
368 keypin_load_journal_impl(const char *data, size_t size,
369                          keypin_journal_pruner_t *pruner)
370 { 
371   const char *start = data, *end = data + size, *next;
372 
373   int n_corrupt_lines = 0;
374   int n_entries = 0; 
375   int n_duplicates = 0;
376   int n_conflicts = 0;
377 
378   for (const char *cp = start; cp < end; cp = next) {
379     const char *eol = memchr(cp, '\n', end-cp);
380     const char *eos = eol ? eol : end;
381     const size_t len = eos - cp;

We should think about this more and make sure this is safe.

Child Tickets

Change History (5)

comment:1 Changed 3 years ago by nickm

Points: .1

Are we okay with "thou shalt not stick a NUL in your text files" as the answer to this one?

comment:2 Changed 3 years ago by isabela

Keywords: isaremoved added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:3 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:4 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:5 Changed 3 years ago by dgoulet

Resolution: wontfix
Status: newclosed

Agree with comment:1 and I can't find a way that a NUL value could get written to the file even with a partial write or write() error...

Note: See TracTickets for help on using tickets.