New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SASL Code Quality and Feature Improvements #598

Merged
merged 119 commits into from Dec 26, 2017

Conversation

Projects
None yet
3 participants
@aaronmdjones
Copy link
Member

aaronmdjones commented Dec 13, 2017

The saslserv/main commits that end in 'move' only move functions around; they don't change any code. This should speed up review.

aaronmdjones added some commits Dec 13, 2017

modules/saslserv/*.c: put function names on their own line
This makes `egrep '^funcname'` possible.
modules/saslserv/external: general code quality cleanups
- Use appropriate qualifiers and attributes on function parameters
- Move variables to where they are used
modules/saslserv/: various adjustments
- Rename sasl_mech_register_func_t to sasl_core_functions_t
  and make it const

- All mechanism providers: use explicit structure initialisation
  for sasl_mechanism_t (named elements)

- All mechanism providers: remove .mech_start and .mech_finish
  function pointers from sasl_mechanism_t where they do nothing

- saslserv/main: only call ->mech_start() and ->mech_finish()
  if they are defined
modules/saslserv/ecdsa-nist256p-challenge: general code quality cleanups
- Use appropriate qualifiers and attributes on function parameters
- Avoid enum and struct typedefs that end in _t as these are reserved
- Move variables to where they are used
- Use arc4random_buf() not RAND_pseudo_bytes(3ssl) for challenge
- Check return value of o2i_ECPublicKey(3ssl)
- Cast signature length parameter on ECDSA_verify(3ssl) correctly
- Use libathemecore/memory instead of mowgli allocator
- Set p->mechdata to NULL after freeing it
modules/saslserv/*.c: move sasl_mechanism_t declaration
This puts them below the functions they point to, so that we
don't need any forward declarations.
modules/saslserv/authcookie: general code quality cleanups
- Use appropriate qualifiers and attributes on function parameters
- Ensure we don't blow up if the input data isn't NULL-terminated
- Use pointer arithmetic to extract client-provided data
modules/saslserv/main: put function names on their own line
This makes `egrep -R '^function_name'` possible.
modules/saslserv/main: fix (dead) potential segfault in moddeinit
All of the SASL mechanism modules import a symbol from the
saslserv/main module when they are loaded, which puts
saslserv/main into their dependency tree.

If saslserv/main is unloaded, all of the modules that depend on
it are unloaded first, and those modules are responsible for
unregistering the mechanism(s) they added. This will result in
any sessions using that mechanism also being destroyed.

If there are still sessions left when saslserv/main is being
unloaded, it must be because one of the mechanism modules did
not unregister itself.

However, the moddeinit function then went on to try and clean up
the sessions itself. The problem is that destroy_session() will
call ->mech_finish() (if it is provided by the module) but the
module that contains that function is now unloaded, so we will
be calling a function that no longer exists.

This isn't actually possible if people don't write their own
SASL mechanism modules, because all of the in-tree modules
correctly unregister themselves when they are being unloaded.

Remove the loop and upgrade the existing warning to a higher log
level so that it is seen by more users.
modules/saslserv/scram-sha: general code quality cleanups
- Rename mechanism functions consistent with other modules
- Move mech_finish() to below mech_step() consistent with other modules
- Remove an unnecessary level of indentation in mech_finish()
- Remove some duplicate code for unregistering mechanisms
PBKDF2v2/SCRAM-SHA: Move shared functions to a shared structure
This avoids numerous pedantic warnings about casting a data
pointer to a function pointer when the SCRAM-SHA module imports
them.
modules/saslserv/plain: general code quality cleanups
- Use appropriate qualifiers and attributes on function parameters
- Ensure we don't blow up if the input data isn't NULL-terminated
- Use pointer arithmetic to extract client-provided data
@aaronmdjones

This comment has been minimized.

Copy link
Member Author

aaronmdjones commented Dec 16, 2017

Apologies for GitHub; it put the 2 aforementioned commits BEFORE my comment.

aaronmdjones added some commits Dec 16, 2017

modules/saslserv/main: sasl_packet: use smaller buffer length
SASL_C2S_MAXLEN is the maximum length of *base64-encoded* data we will
process, so we don't need a buffer larger than this for encoding.
modules/saslserv/main: mechlist_build_string: more cleanup
- More const-correctness
- Rename a 1-letter variable
- Calculate mechanism name length once instead of 4 times
- Avoid potential buffer overrun (incorrect length comparison logic)
- This function only ever writes to 1 buffer so just hardcode that
modules/saslserv/*.c: add new kind of start/step mech error code
ASASL_FAIL results in a bad_password() on the user, even if the
failure was due to some kind of internal error (e.g. too small a
buffer passed to base64_encode()), which is undesirable.

Add a new code (ASASL_ERROR) that aborts the sessions all the
same but does not bad_password() the user, and reserve ASASL_FAIL
only for cases where the client supplied correctly-formatted data
but nonetheless invalid credentials.
modules/saslserv/main: fix use-after-free & double-free
Clang's static analyzer identified the following problematic call chain
introduced in this branch:

  sasl_input()
    p = find_or_make_session()
    sasl_buf_process(p)
      sasl_packet(p)
        sasl_session_abort(p)
          destroy_session(p)
            free(p->buf)
            free(p)
      free(p->buf)

This would have been remotely exploitable.

Fix the issue as follows:

* `sasl_packet()` and `sasl_buf_process()` will return a failure code

* `sasl_packet()` and `sasl_buf_process()` are annotated with the
  `warn_unused_result` function attribute to ensure this failure is
  propagated in the future (and remove the possibility of leaking memory)

* `sasl_input()` will abort/destroy the session if `sasl_packet()` fails

* `sasl_session_abort()` and `destroy_session()` are moved to below
  `sasl_buf_process()` and `sasl_packet()` so that these 2 functions can't
  abort or destroy sessions any more (there are no forward declarations)

This commit should serve as a reminder of the usefulness of static
analysis tools.
modules/saslserv/main: break out complex logic to separate functions
This makes the changes introduced in the previous commit less ugly.
modules/saslserv/main: move 'struct sasl_sourceinfo' to shared header
Some users (e.g. Freenode) have created modules that hook user_can_login
and use various pieces of SASL information by casting the sourceinfo
back to this structure.

Such modules would need a duplicate definition which is likely to
introduce problems and incompatibilities in the future. Make this
impossible by making the definition shared among the entire codebase.
@grawity

This comment has been minimized.

Copy link
Contributor

grawity commented on cf3473f Dec 21, 2017

Maybe random_string() should then have a note that some callers are relying on its output being Base64-compatible? (In case somebody later decides to expand its character range.)

This comment has been minimized.

Copy link
Member Author

aaronmdjones replied Dec 21, 2017

RFC 5802 (SCRAM-SHA) requirements for the server nonce are only that it is printable (any printable ASCII character except a comma).

This comment has been minimized.

Copy link
Contributor

grawity replied Dec 21, 2017

Ah, d'oh.

@ilbelkyr
Copy link
Member

ilbelkyr left a comment

Looks good to me, though two things stand out a little – the "Account ... dropped, login cancelled" message is the actually important one to me, though.

if (! mu)
{
(void) notice(saslsvs->nick, u->nick, "Account %s dropped, login cancelled",
p->authzid ? p->authzid : "??");
p->authzeid ? p->authzeid : "??");

This comment has been minimized.

@ilbelkyr

ilbelkyr Dec 23, 2017

Member

As remarked on IRC, this should possibly indicate the client-specified authzid instead. We would probably need to keep track of it alongside the EID for logging purposes.

}

static bool
sasl_authzid_can_login(struct sasl_session *const restrict p, const char *const restrict authzid,

This comment has been minimized.

@ilbelkyr

ilbelkyr Dec 23, 2017

Member

I'll admit the code duplication between sasl_authzid_can_login and sasl_authcid_can_login makes me a little unhappy.

@ilbelkyr

This comment has been minimized.

Copy link
Member

ilbelkyr commented Dec 23, 2017

(Not really related to this PR, but something I thought of while reviewing it – doesn't saslserv/scram-sha need to be taught about the MU_NOPASSWORD flag?)

aaronmdjones added some commits Dec 23, 2017

@ilbelkyr ilbelkyr merged commit 45a7f19 into master Dec 26, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aaronmdjones aaronmdjones deleted the sasl-fixups branch Dec 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment