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

Problems: free of stack variable, TODO left to evaluate #2700

Merged
merged 9 commits into from
Aug 19, 2017

Conversation

bluca
Copy link
Member

@bluca bluca commented Aug 18, 2017

Solutions: see commits

Fixes #2701 #2699

Solution: free wrapper.value instead of wrapper.
Solution: remove it.
Looking at the code:

https://github.com/krb5/krb5/blob/master/src/lib/gssapi/mechglue/g_unseal.c#L55

gss_unwrap as the very first thing checks that plaintext is not a
null pointer, which in our case it's true given it's on the stack,
and then initialises its members to 0 length and null ptr.

https://github.com/krb5/krb5/blob/master/src/lib/gssapi/mechglue/g_rel_buffer.c#L36

So it should be safe to release it in all cases, and the release API
seems to check again if it's not a null pointer and then if the
members are 0 length and null pointer it's a no-op.
@sigiesec
Copy link
Member

@bluca, thanks for correcting my mistake in the call to free. It is quite hard to modify the source without having a chance to run or even compile it.

@bluca
Copy link
Member Author

bluca commented Aug 18, 2017

No problem!

Solution: use it before falling back to headers checks
Solution: remove the second one to fix build failure
Solution: monitor new events only if DRAFT APIs are enabled, and
convert to new event types. Same for DRAFT socket options.
@bluca
Copy link
Member Author

bluca commented Aug 18, 2017

@sigiesec if you are on Linux, with the changes in this PR you can install the library and build with it:

sudo apt install libkrb5-dev
./configure .... --with-libgssapi_krb5=yes

It won't run the test unless you setup the whole kerberos thing, but at least you can check it builds.

I'll see if I can add it by default to the packages builds on OBS, and then on a couple of CI jobs as well that already install packages.

int event = get_monitor_event (server_mon, NULL, NULL);
assert (event == ZMQ_EVENT_HANDSHAKE_FAILED);
assert (event == ZMQ_EVENT_HANDSHAKE_FAILED_AUTH);
Copy link
Member Author

Choose a reason for hiding this comment

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

@sigiesec I took a very wild guess here and in the next events, is this correct?

Copy link
Member

Choose a reason for hiding this comment

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

This one looks good.

@bluca
Copy link
Member Author

bluca commented Aug 18, 2017

There's a few errors in the GSSAPI code with older GCC on Debian 7, Ubuntu 12.04 and RHEL 7. I'll have a look during the weekend:

https://build.opensuse.org/package/live_build_log/home:bluca:branches:network:messaging:zeromq:git-draft/libzmq/Debian_7.0/x86_64

Solution: as discussed, remove the deprecation notices, as many users
expressed the need for multi-part support.
Fixes zeromq#2699
Solution: add it to CMake's platform.hpp.in
Solution: use LIBZMQ_UNUSED where necessary
Solution: add missing target_link_libraries
Fixes zeromq#2701
@somdoron somdoron merged commit 2c8a131 into zeromq:master Aug 19, 2017
@bluca bluca deleted the gssapi_uninit_ref branch August 19, 2017 11:51
int event = get_monitor_event (server_mon, NULL, NULL);
assert (event == ZMQ_EVENT_HANDSHAKE_FAILED);
assert (event == ZMQ_EVENT_HANDSHAKE_FAILED_AUTH);
Copy link
Member

Choose a reason for hiding this comment

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

This one looks good.

int event = get_monitor_event (server_mon, NULL, NULL);
assert (event == ZMQ_EVENT_HANDSHAKE_FAILED);
assert (event == ZMQ_EVENT_HANDSHAKE_FAILED_AUTH);
Copy link
Member

Choose a reason for hiding this comment

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

This should be ZMQ_EVENT_HANDSHAKE_FAILED_PROTOCOL with value ZMQ_PROTOCOL_ERROR_ZMTP_MECHANISM_MISMATCH as in

ZMQ_EVENT_HANDSHAKE_FAILED_PROTOCOL,

Maybe expect_zmtp_mechanism_mismatch should be moved to testutil_security.hpp and reused here (and for the PLAIN client test as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, sent a quick fix here: #2706

// Monitor handshake events on the server
rc = zmq_socket_monitor (server, "inproc://monitor-server",
ZMQ_EVENT_HANDSHAKE_SUCCEED | ZMQ_EVENT_HANDSHAKE_FAILED);
ZMQ_EVENT_HANDSHAKE_SUCCEEDED | ZMQ_EVENT_HANDSHAKE_FAILED_AUTH);
Copy link
Member

Choose a reason for hiding this comment

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

Here, ZMQ_EVENT_HANDSHAKE_FAILED_PROTOCOL must also be enabled.

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.

3 participants