~kevin8t8/mutt

ref: 564af5129c2e3de82d7d9ed80dcbbd55d482b435 mutt/imap/imap.c -rw-r--r-- 68.8 KiB
Merge branch 'stable'
Ensure idata->check_status is cleared on mailbox close.

I don't think this would cause any issues, but it should be cleared
here in any case.
Clean up labels in imap_open_connection().

Commit 04b06aaa was purposely kept minimal, to aid backporting the
fix.  It kept the err_close_conn label, but there is no need for the
label anymore.  Change all goto's to use the bail label.
Merge branch 'stable'
Ensure IMAP connection is closed after a connection error.

During connection, if the server provided an illegal initial response,
Mutt "bailed", but did not actually close the connection.  The calling
code unfortunately relied on the connection status to decide to
continue with authentication, instead of checking the "bail" return
value.

This could result in authentication credentials being sent over an
unencrypted connection, without $ssl_force_tls being consulted.

Fix this by strictly closing the connection on any invalid response
during connection.  The fix is intentionally small, to ease
backporting.  A better fix would include removing the 'err_close_conn'
label, and perhaps adding return value checking in the caller (though
this change obviates the need for that).

This addresses CVE-2020-28896.  Thanks to Gabriel Salles-Loustau for
reporting the problem, and providing test cases to reproduce.
Remove always true conditional (#if 1)
Directly add/remove mailboxes for IMAP.

The LSUB processor along with <subscribe> and <unsubscribe> would
construct a mailboxes command and send it to the muttrc parser.

Since the poll/label refactor it's much easier to just directly call
the functions, and is much more secure.  So do that instead.
Add $tunnel_is_secure config, defaulting set.

The config variable is to resolve an ambiguity in Mutt about whether
using $tunnel is secure.

On the one hand, the examples in the manual show using ssh or a direct
pipe to a program.  Many users do this to connect to an IMAP server
with PREAUTH configured, relying on the tunnel to be secured by ssh or
by the fact that it's a local pipe.

On the other hand, the Mutt connection code still respects
$ssl_starttls and $ssl_force_tls, as if the $tunnel connection were
not already secured.

After some discussion on mutt-dev, it seemed the best idea to assume
the connection is secure by default, in order to not break IMAP
PREAUTH connections, but to provide a configuration variable in case
there are situations where it is not.

Thanks to Aaron Schrab for the original idea of setting conn->ssf for
$tunnel in his patch to ticket 250.
Merge branch 'stable'
Remove $ssl_starttls check for IMAP PREAUTH.

Checking $ssl_starttls provides no real protection, because an
attacker can just as easily spoof "* OK" and strip the STARTTLS
capability as it can spoof "* PREAUTH".  The only way to really
protect again the MITM is through $ssl_force_tls.

Add documentation about STARTTLS, $tunnel, and the current PREAUTH
exception when using $tunnel.

The behavior of Mutt about $tunnel is somewhat inconsistent: is it
considered secure or not?  For PREAUTH, to avoid breaking
configurations, we assume it is secure.  But at the same time, Mutt is
still negotiating STARTTLS for other $tunnel connections.

This will be resolved in master for the next release; probably by
adding a $tunnel_is_secure config variable defaulting "yes" and
removing the STARTTLS negotiation in that case.
Merge branch 'stable'
Don't check IMAP PREAUTH encryption if $tunnel is in use.

$tunnel is used to create an external encrypted connection.  The
default of $ssl_starttls is yes, meaning those kinds of connections
will be broken by the CVE-2020-14093 fix.
Add L10N comment for unencrypted PREAUTH warning.

I forgot to add one while trying to get the fix out.
Merge branch 'stable'
Prevent possible IMAP MITM via PREAUTH response.

This is similar to CVE-2014-2567 and CVE-2020-12398.  STARTTLS is not
allowed in the Authenticated state, so previously Mutt would
implicitly mark the connection as authenticated and skip any
encryption checking/enabling.

No credentials are exposed, but it does allow messages to be sent to
an attacker, via postpone or fcc'ing for instance.

Reuse the $ssl_starttls quadoption "in reverse" to prompt to abort the
connection if it is unencrypted.

Thanks very much to Damian Poddebniak and Fabian Ising from the
Münster University of Applied Sciences for reporting this issue, and
their help in testing the fix.
Try to automatically reconnect to an open IMAP mailbox on error.

For the Context, change cmd_handle_fatal() to flag idata->status on
the error, but not close the mailbox and disconnect.

Then have imap_check_mailbox() reconnect when IMAP_REOPEN_ALLOW is
set (so the index is prepared to rebuild with all new headers).  Try
to merge flag, envelope, and attach_del changes back in.  Replace the
existing Context pointer with the new values, and swap out the idata
passed in on success, so we don't continue to work with the wrong
idata.

The imap_check_mailbox() changes are purposely coded loosely to try
and catch all sorts of errors - not just polling and imap_idle
errors.

Create a new check_mailbox() return code to indicate a reconnect.
Display a more appropriate message in the index, but otherwise treat
it the same as a REOPEN.
Ensure idata->reopen is clear when the mailbox is closed.

Commit ab457327 accidentally introduced a performance regression in
the 1.13.0 release.  The reason is that imap_sync_mailbox() was
leaving IMAP_REOPEN_ALLOW set.  When a mailbox was closed, and another
opened, the imap_open_mailbox() would be processed with the reopen
set.

The initial connection and exec statements would trigger a
imap_cmd_finish().  That would then prematurely trigger fetching new
mail, with the newMailCount misset, which would in turn generate a
tiny uid_hash.  When the real download occurred, it would use the
existing tiny hash, throttling performance.

The regression commit removed what I thought was an unnecessary check,
but it was guarding the fetch from happening at the wrong time during
mailbox open too.

The performance issue was already addressed by commit a8337951 in
master, but I think it's good to wipe the entire state.  The removes
other possible strange behavior.

A HUGE thanks to Guilhem Moulin for his help on this issue.  Without
all his work detailing the problem, creating automated testing, git
bisecting, and logging, I would not have been able to find and fix the
problem.
Turn off REOPEN flag after imap open and sync operations.

Leaving a just opened imap mailbox with REOPEN set can cause all sorts
of problems.  Postpone.c had a memory corruption bug because of this.
The background compose reply-flag setting code had to ensure it was
turned off too.

The main index worked around this by always ensuring
mx_check_mailbox() was called right afterwards.  But after researching
I can't see any reason to leave it open.

imap_sync_mailbox() also leaves the REOPEN set, and I can't see a good
reason for doing this either.  Both are memory corruption bugs just
waiting to happen.

With the flag turned off, remove the workaround calls to
mx_check_mailbox() in postpone.c and send.c.
Convert mutt_parse_rc_line() to use real buffer to hold the line.

Previously, the parameter converted into a kind of read-only buffer,
pointing to the char *line passed in.  The problem is that
mutt_extract_token() backtick processing allocates and assigns a new
token->data.

As a result, buffer->destroy was added, whose sole purpose is to keep
track of the read-only token buffer being reallocated so that it can
be properly freed inside mutt_parse_rc_line().  (The char *line is
allocated and freed in source_rc()).

Remove the token parameter, and convert line to a const char *.  Copy
that into a buffer-pool buffer, so the buffer can be modified
"normally".

Create a mutt_buffer_rc_buffer() function taking the line buffer
parameter.  To make the source_rc() just a tiny bit faster, have it
directly manage and call mutt_parse_rc_buffer() itself.

Other callers likely don't need the speed, so remove their cumbersome
creation/free of that parameter.

The next commit will deal with removing buffer->destroy and fixing up
backtick handling.
Ensure rc_line err BUFFER is always dynamic.

The mailboxes command changes started using standard mutt_buffer_*()
functions, which will attempt to realloc err->data if the size is too big.
Add -label and -nopoll arguments to mailboxes command

-nopoll allows adding the mailbox to the sidebar, or the mailbox
browser menu, without incurring polling costs.

-poll can be used to re-enable polling for a previously disabled
mailbox.

-label allows specifying an alternative string to show in the sidebar
and mailbox browser menu.

-nolabel removes a label from an existing mailbox.

Change $sidebar_sort_method so that "alpha" and "name" will sort by
the label if specified.  "path", however, will always sort by the
actual mailbox path.
Next