Opened 4 years ago

Closed 4 years ago

#16582 closed defect (implemented)

Distinguish ENOENT from other error cases when loading keys.

Reported by: nickm Owned by:
Priority: Very High Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: TorCoreTeam201507
Cc: Actual Points:
Parent ID: #16530 Points:
Reviewer: Sponsor:

Description

You know what's horrible? Trying to load a key, failing with EMFILE or something, and then concluding that the key doesn't exist, and then overwriting it.

We'd better only ever say "No key, better make one" when we get ENOENT.

Child Tickets

Change History (11)

comment:1 Changed 4 years ago by teor

This affects the file_status function. I last worked on file_status in #13111 to resolve an issue where tor wouldn't start if key files were present but zero-length.

I've just reviewed all the key-loading code which uses file_status again.

I can't see how this abberant overwriting behaviour occurs, based on reading the latest code. Keys are only replaced on FN_NOENT (ENOENT), and, in many cases, FN_EMPTY (a zero-length file).

But if EMFILE occurs (or any errno other than ENOENT), file_status returns FN_ERROR, and key loading fails. Nothing is read, generated, or overwritten.

Are we loading keys without checking file_status?
Which particular key-loading code or type of key exhibits this issue?

comment:2 Changed 4 years ago by nickm

Status: newneeds_review

feature_16582 should fix this one for ed25519 keys.

comment:3 Changed 4 years ago by nickm

teor: I'm mainly concerned with ed25519 keys here; I believe they don't use file_status.

comment:4 Changed 4 years ago by teor

That makes much more sense - I've just checked, and the ed25519 code doesn't use file_status.

In particular, ed_key_init_from_file and load_ed_keys assume every file loading error is an ENOENT, and go and create new keys (and certificates!)

Is the plan to make this consistent with the key-handling code for other key types?
In particular, after #13111, the following cases cause keys to be created:

  • ENOENT - an absent key file (file_status FN_NOENT)
  • a zero-length key file (file_status FN_EMPTY)

comment:5 Changed 4 years ago by nickm

Priority: majorcritical

comment:6 Changed 4 years ago by nickm

I think consistency is a later thing -- right now the most critical thing IMO is to never treat EMFILES as a cue to replace our master key.

comment:7 Changed 4 years ago by teor

I think we want errno to always be set on error.
Because this is what ed_key_init_from_file assumes in 5e8edba3d80bf53e5e5c09c8a87e06d0c69e00b7

To make this happen in b566cb9e84b095289a1c662e953218c9a7d1f77d

In read_file_to_str
This code should probably set errno = EINVAL

  if ((uint64_t)(statbuf.st_size)+1 >= SIZE_T_CEILING) {
    close(fd);
    return NULL;
  }

To match these changes in crypto_read_tagged_contents_from_file:

if (st.st_size < 32 || st.st_size > 32 + data_out_len) {
  saved_errno = EINVAL;
  goto end;
}

Similarly, in read_all, the following code should probably set errno = EINVAL:

  if (count > SIZE_T_CEILING || count > SSIZE_MAX)
    return -1;

And in read_file_to_str_until_eof:

  if (max_bytes_to_read+1 >= SIZE_T_CEILING)
    return NULL;

To make this happen in 0a6997d78bdbf485f42acfa95558a91db3381d70

In read_encrypted_secret_key, the following code should probably set errno = EINVAL:

  if (strcmp(tag, ENC_KEY_TAG))
    goto done;
    if (pwlen < 0)
      goto done;

In ed25519_seckey_read_from_file, the following code should probably set errno = EINVAL:

  if (len != sizeof(seckey_out->seckey))
    return -1;

Similarly, in ed25519_pubkey_read_from_file:

  if (len != sizeof(pubkey_out->pubkey))
    return -1;

In 5e8edba3d80bf53e5e5c09c8a87e06d0c69e00b7:

Do we want to implement INIT_ED_KEY_NO_REPAIR for the certificate file, or is replacing it on any error ok?

comment:8 Changed 4 years ago by nickm

Keywords: TorCoreTeam201507 added

comment:9 Changed 4 years ago by nickm

feature_16582 now has fixes for the above issues. Do you like it now?

comment:10 Changed 4 years ago by teor

Looks good now. Very errnoeous.

comment:11 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Great; merged!

Note: See TracTickets for help on using tickets.