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

UnboundID SDK catches throwable and rewraps hiding a fatal error #33175

Closed
jaymode opened this issue Aug 27, 2018 · 3 comments · Fixed by #34247
Closed

UnboundID SDK catches throwable and rewraps hiding a fatal error #33175

jaymode opened this issue Aug 27, 2018 · 3 comments · Fixed by #34247
Assignees
Labels
>bug :Security/Security Security issues without another label

Comments

@jaymode
Copy link
Member

jaymode commented Aug 27, 2018

As with some other libraries used in our codebase, the unboundid ldap sdk catches throwable in places and re-wraps what could be a potentially fatal error in an LDAPException:

https://github.com/pingidentity/ldapsdk/blob/0eac394a077323485b850a90e55f926bc035023f/src/com/unboundid/ldap/sdk/LDAPConnectionPool.java#L1623-L1675

We should raise an issue with the upstream library to ask that they stop catching throwable or at least rethrow properly. In the interim we should use ExceptionsHelper#maybeError with these exceptions so that we can properly die if an unrecoverable error is being hidden.

@jaymode jaymode added >bug :Security/Security Security issues without another label labels Aug 27, 2018
@jaymode jaymode self-assigned this Aug 27, 2018
@jasontedor
Copy link
Member

😱

@jaymode
Copy link
Member Author

jaymode commented Aug 27, 2018

I opened pingidentity/ldapsdk#49 for the library maintainers' consideration; PingIdentity does not accept third party contributions for the ldapsdk so this is not something we could submit a patch for.

@jaymode
Copy link
Member Author

jaymode commented Aug 28, 2018

This has been addressed in the upstream library by commit pingidentity/ldapsdk@9b012d2. We'll still want a stop-gap until a version with said commit is released and we upgrade to it.

jaymode added a commit to jaymode/elasticsearch that referenced this issue Oct 2, 2018
This commit upgrades the unboundid ldapsdk to version 4.0.8. The
primary driver for upgrading is a fix that prevents this library from
rewrapping Error instances that would normally bubble up to the
UncaughtExceptionHandler and terminate the JVM. Other notable changes
include some fixes related to connection handling in the library's
connection pool implementation.

Closes elastic#33175
jaymode added a commit that referenced this issue Oct 3, 2018
This commit upgrades the unboundid ldapsdk to version 4.0.8. The
primary driver for upgrading is a fix that prevents this library from
rewrapping Error instances that would normally bubble up to the
UncaughtExceptionHandler and terminate the JVM. Other notable changes
include some fixes related to connection handling in the library's
connection pool implementation.

Closes #33175
jaymode added a commit that referenced this issue Oct 3, 2018
This commit upgrades the unboundid ldapsdk to version 4.0.8. The
primary driver for upgrading is a fix that prevents this library from
rewrapping Error instances that would normally bubble up to the
UncaughtExceptionHandler and terminate the JVM. Other notable changes
include some fixes related to connection handling in the library's
connection pool implementation.

Closes #33175
kcm pushed a commit that referenced this issue Oct 30, 2018
This commit upgrades the unboundid ldapsdk to version 4.0.8. The
primary driver for upgrading is a fix that prevents this library from
rewrapping Error instances that would normally bubble up to the
UncaughtExceptionHandler and terminate the JVM. Other notable changes
include some fixes related to connection handling in the library's
connection pool implementation.

Closes #33175
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Security/Security Security issues without another label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants