Skip to content
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

Improve sshbuf defensive measures #287

Closed
wants to merge 2 commits into from

Conversation

stoeckmann
Copy link

  • Keep track of reference count even if parent is set multiple times
  • Gracefully handle failed re-allocation in sshbuf_reset

Shoutout to @c3h2_ctf

This situation cannot be reached currently, so it should be a
purely defensive measure.
If recallocarray fails in sshbuf_reset, then it could happen that
SSHBUF_SIZE_INIT bytes are not available, which would lead to an
out of boundary access with explicit_bzero.

Check return value of sshbuf_check_sanity to comply with other
sshbuf functions and gracefully handle failed re-allocation.
@@ -109,6 +109,8 @@ sshbuf_set_parent(struct sshbuf *child, struct sshbuf *parent)
if ((r = sshbuf_check_sanity(child)) != 0 ||
(r = sshbuf_check_sanity(parent)) != 0)
return r;
if (child->parent != NULL)
child->parent->refcount--;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes sense for a child buffer to be "re-parented".

IMO we it would be better to explicitly disallow this, e.g.

        if (child->parent != NULL && child->parent != parent)
                return SSH_ERR_INTERNAL_ERROR;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in that case it makes sense to treat it as internal error.

@djmdjm
Copy link
Contributor

djmdjm commented Apr 8, 2022

Thanks - these have been applied and will be in OpenSSH 9.1

@djmdjm djmdjm closed this Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants