Skip to content

Stabilize closing mechanism for EncWriter and DecWriter #17

Open
@aead

Description

@aead

Background Information

The en/decryption of the last fragment differs from all previous fragments since the
first bit of the associated data is flipped from 0 to 1 (i.e. aad[0] |= 0x80). Now, readers and
writers must somehow detect whether the entire plaintext/ciphertext stream has been received -
we don't know whether the (ciphertext) data is authentic but we need to somehow detect the end-of-stream symbol to trigger the "special" logic for the last fragment.

For readers we can assume an EOF after Ok(0). However, for writers the caller has to signal "somehow" that no more data will be written. Therefore, some "close"/"flush" mechanism is necessary.

How to signal an end-of-stream for EncWriter and DecWriter

In general, there are two main options:

  1. An end-of-stream is signaled by calling the flush method defined by std::io::Write.
    Optionally, a flush can happen in the destructor - again like BufWriter.
  2. We can define a separate close method that signales the end-of-stream.
    Optionally, the close can happen (if not called explicitly) in the destructor.

Drop behavior

Invoking the "close" mechanism (if not invoked explicitly) does not seem to be correct.

  • EncWriter:
    If the EncWriter is "closed" implicitly (during drop) it is not possible to handle any error
    that happens during writing the final fragment to the underlying writer - the only option would
    be to panic. Not handling the error may cause silent data corruption since an incomplete
    ciphertext cannot be distinguished from a maliciously truncated one, and therefore, cannot be
    decrypted successfully & securely. However, panic'ing when writing to the underlying writer
    fails introduces non-deterministic program failures. In general, the destructor should not
    panic.
  • DecWriter:
    The same is true for DecWriter. In addition, "closing" during drop also fails if the ciphertext
    stream is not authentic. Not handling that error causes incomplete plaintexts such that an
    attacker can mount truncation attacks against programs that use the implicit "close" - either
    intentionally or accidentally. On the other side, panic'ing in that case gives an attacker the
    ability to mount DoS attacks against such programs.

Therefore:

  1. There should be no (implicit) "close" mechanism when dropped.
  2. The "close" mechanism must be invoked explicitly by the caller.

Now, given that the "close" mechanism must be invoked explicitly, not doing so is a logic bug. Therefore, I suspect that the "correct" behavior (even though unconventional and maybe controversial) is to panic when an EncWriter or DecWriter gets dropped but hasn't been "closed" explicitly. More general, when not "closed" and no previous write call failed.

"Close" mechanisms

Using flush

Initially flush may seem to be an appropriate way to trigger the en/decryption of the last fragment. However, there are the following issues:

  • flush takes a (mut) reference to self and therefore it's possible to write any combination
    of write and flush calls - e.g.:
    w.write(buf)?;
    w.flush()?;
    w.write(buf)?;  // This must fail b/c `flush` triggered the last fragment
    
    While this is completely fine for e.g. BufWriter any write that happens after flush must fail
    for EncWriter or DecWriter. Actually, trying to write more data to a "closed" writer is a logic
    bug.
    To prevent this we would need a method takes ownership of the writer - e.g.
    flush(mut self) -> io::Result<()> or close(mut self) -> io::Result<()>.

Alternatives

There are some alternatives to flush. However, there is the fundamental problem that we need to "close" a chain of internal writers since io::Write should be composable:

  • A method close(mut self) -> io::Result<()>. However, it would not be possible to invoke
    it on any inner writer since it does not implement a close method.
  • A Close trait with a close(mut self) -> io::Result<()> method. Still, this only works
    for the top level EncWriter / DecWriter (if we write a custom Drop implementation).
  • A Close trait with a close(&mut self) -> io::Result<()> method and a NopCloser type
    that wraps any type that does not implement Close. So far that's the most flexible
    approach since callers can provide custom implementations of Close such that we can have
    a chain of close calls. However, a caller can still e.g. write to EncWriter / DecWriter after
    close. We could define a separate close(mut self) -> io::Result<()> method for EncWriter
    and DecWriter such that specialization will select the close that takes ownership. However,
    callers can still do the wrong thing by calling Close::close(w) and than write to w. The
    Close trait documentation can also help by indicating the Close should be implemented but
    never directly used. Anyway, even though the incorrect example may be artificial an ideal
    solution would be to proof in the type system that such an error cannot be made by
    callers.

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-decisionThere are several options possible

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions