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

Scripts: Fix security for deprecation warning #28485

Merged
merged 3 commits into from
Feb 3, 2018

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Feb 1, 2018

If you call getDates() on a long or date type field add a deprecation
warning to the response and log something to the deprecation logger.
This mostly worked just fine but if the deprecation logger happens to
roll then the roll will be performed with the script's permissions
rather than the permissions of the server. And scripts don't have
permissions to, say, open files. So the rolling failed. This fixes that
by wrapping the call the deprecation logger in doPriviledged.

This is a strange doPrivileged call because it doens't check
Elasticsearch's SpecialPermission. SpecialPermission is a permission
that no-script code has and that scripts never have. Usually all
doPrivileged calls check SpecialPermission to make sure that they
are not accidentally acting on behalf of a script. But in this case we
are intentionally acting on behalf of a script.

Closes #28408

If you call `getDates()` on a long or date type field add a deprecation
warning to the response and log something to the deprecation logger.
This *mostly* worked just fine but if the deprecation logger happens to
roll then the roll will be performed with the script's permissions
rather than the permissions of the server. And scripts don't have
permissions to, say, open files. So the rolling failed. This fixes that
by wrapping the call the deprecation logger in `doPriviledged`.

This is a strange `doPrivileged` call because it doens't check
Elasticsearch's `SpecialPermission`. `SpecialPermission` is a permission
that no-script code has and that scripts never have. Usually all
`doPrivileged` calls check `SpecialPermission` to make sure that they
are not accidentally acting on behalf of a script. But in this case we
are *intentionally* acting on behalf of a script.

Closes elastic#28408
@nik9000 nik9000 added review :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v7.0.0 v6.3.0 labels Feb 1, 2018
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Looks good, just one minor question.

@@ -177,15 +206,27 @@ public int size() {
private static final ReadableDateTime EPOCH = new DateTime(0, DateTimeZone.UTC);

private final SortedNumericDocValues in;
private final Consumer<String> deprecationCallback;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this abstracted? Can't the Longs.deprecated method just call deprecationLogger.deprecated directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could but then I couldn't test this. There isn't any consistent way to make it fail without this funny unit test dance that I do because it usually only fails when you rotate the log files. At least, that is the only time I could get it to fail locally.

@nik9000 nik9000 merged commit 5003ef1 into elastic:master Feb 3, 2018
@nik9000
Copy link
Member Author

nik9000 commented Feb 3, 2018

Thanks for reviewing @rjernst!

nik9000 added a commit that referenced this pull request Feb 4, 2018
If you call `getDates()` on a long or date type field add a deprecation
warning to the response and log something to the deprecation logger.
This *mostly* worked just fine but if the deprecation logger happens to
roll then the roll will be performed with the script's permissions
rather than the permissions of the server. And scripts don't have
permissions to, say, open files. So the rolling failed. This fixes that
by wrapping the call the deprecation logger in `doPriviledged`.

This is a strange `doPrivileged` call because it doens't check
Elasticsearch's `SpecialPermission`. `SpecialPermission` is a permission
that no-script code has and that scripts never have. Usually all
`doPrivileged` calls check `SpecialPermission` to make sure that they
are not accidentally acting on behalf of a script. But in this case we
are *intentionally* acting on behalf of a script.

Closes #28408
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Jan 9, 2019
When the deprecation log is written to within scripting support code
like ScriptDocValues, it runs under the reduces privileges of scripts.
Sometimes this can trigger log rolling, which then causes uncaught
security errors, as was handled in elastic#28485. While doing individual
deprecation handling within each deprecation scripting location is
possible, there are a growing number of deprecations in scripts.

This commit wraps the logging call within the deprecation logger use a
doPrivileged block, just was we would within individual logging call
sites for scripting utilities.
rjernst added a commit that referenced this pull request Jan 10, 2019
…37281)

When the deprecation log is written to within scripting support code
like ScriptDocValues, it runs under the reduces privileges of scripts.
Sometimes this can trigger log rolling, which then causes uncaught
security errors, as was handled in #28485. While doing individual
deprecation handling within each deprecation scripting location is
possible, there are a growing number of deprecations in scripts.

This commit wraps the logging call within the deprecation logger use a
doPrivileged block, just was we would within individual logging call
sites for scripting utilities.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v6.3.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ScriptDocValues getDate() on long field triggers AccessControlException
4 participants