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.

Sign up for GitHub

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

Jump to bottom

HLRC: Fix '+' Not Correctly Encoded in GET Req. #33164

Merged
merged 24 commits into from
Jul 15, 2019
Merged

HLRC: Fix '+' Not Correctly Encoded in GET Req. #33164

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d607643
HLRC: Fix '+' Not Correctly Encoded in GET Req.
original-brownbear Aug 27, 2018
127aa1c
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear Aug 27, 2018
f3e9208
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear Aug 27, 2018
c16be99
Remove server side handling of + as ' '
original-brownbear Aug 27, 2018
ce12e8d
fix test
original-brownbear Aug 27, 2018
e9862d9
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear Aug 27, 2018
906d51e
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear Sep 3, 2018
9da77eb
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear Nov 25, 2018
33ac4c8
CR: Add system property for decoding + as space or not
original-brownbear Nov 25, 2018
88661c1
fix typo
original-brownbear Nov 25, 2018
1d59ba8
hack around encoding asymetry in saml
original-brownbear Nov 25, 2018
20ee2f8
Merge remote-tracking branch 'elastic' into 33077
original-brownbear Nov 26, 2018
4c65f6b
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear Jan 5, 2019
9b7b010
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear May 29, 2019
d32bee1
fix test
original-brownbear May 29, 2019
1704338
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear May 30, 2019
e4397d1
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear Jun 23, 2019
0f8be02
CR: Add deprecation/breaking changes note
original-brownbear Jun 23, 2019
c792bda
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear Jun 23, 2019
357e801
Keep encoding + as space in query strings
original-brownbear Jun 23, 2019
fbe3323
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear Jun 24, 2019
d0f38a1
correctly encode jobid in test
original-brownbear Jun 24, 2019
d34ae83
Merge remote-tracking branch 'elastic/master' into 33077
original-brownbear Jun 24, 2019
94b2ba9
CR:revert SAML change
original-brownbear Jun 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,8 @@ class BuildPlugin implements Plugin<Project> {
// TODO: remove this once ctx isn't added to update script params in 7.0
systemProperty 'es.scripting.update.ctx_in_params', 'false'

systemProperty 'es.rest.url_plus_as_space', 'false'

// Set the system keystore/truststore password if we're running tests in a FIPS-140 JVM
if (project.inFipsJvm) {
systemProperty 'javax.net.ssl.trustStorePassword', 'password'
Expand Down
1 change: 1 addition & 0 deletions client/rest-high-level/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ integTestRunner {

integTestCluster {
systemProperty 'es.scripting.update.ctx_in_params', 'false'
systemProperty 'es.rest.url_plus_as_space', 'false'
setting 'xpack.license.self_generated.type', 'trial'
setting 'xpack.security.enabled', 'true'
setting 'xpack.security.authc.token.enabled', 'true'
Expand Down
20 changes: 20 additions & 0 deletions client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1111,6 +1111,26 @@ public void testParamsEncode() throws IOException {
}
}

public void testGetIdWithPlusSign() throws Exception {
String id = "id+id";
{
IndexRequest indexRequest = new IndexRequest("index", "type", id);
indexRequest.source("field", "value");
IndexResponse indexResponse = highLevelClient().index(indexRequest, RequestOptions.DEFAULT);
assertEquals("index", indexResponse.getIndex());
assertEquals("type", indexResponse.getType());
assertEquals(id, indexResponse.getId());
}
{
GetRequest getRequest = new GetRequest("index", "type", id);
GetResponse getResponse = highLevelClient().get(getRequest, RequestOptions.DEFAULT);
assertTrue(getResponse.isExists());
assertEquals("index", getResponse.getIndex());
assertEquals("type", getResponse.getType());
assertEquals(id, getResponse.getId());
}
}

// Not entirely sure if _termvectors belongs to CRUD, and in the absence of a better place, will have it here
public void testTermvectors() throws IOException {
final String sourceIndex = "index1";
Expand Down
9 changes: 8 additions & 1 deletion server/src/main/java/org/elasticsearch/rest/RestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.elasticsearch.rest;

import org.elasticsearch.common.Booleans;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.path.PathTrie;

Expand All @@ -30,6 +31,12 @@

public class RestUtils {

/**
* Sets whether we decode a '+' in an url as a space or not.
*/
private static final boolean DECODE_PLUS_AS_SPACE =
Booleans.parseBoolean(System.getProperty("es.rest.url_plus_as_space", "true"));
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should immediately deprecate a true value here. Then in 8.0.0 we can reject true, allow for unset or false (with the latter deprecated), and then finally remove in 9.0.0 (let’s add assertions on the major version for this). Let us also add a note to the migration guide. That means after this PR is merged to master and backported to 7.x (only), we need a follow-up in master to reject false and do the deprecation and add the assertion.


public static final PathTrie.Decoder REST_DECODER = new PathTrie.Decoder() {
@Override
public String decode(String value) {
Expand Down Expand Up @@ -164,7 +171,7 @@ private static int decode(String s, int size, byte[] buf) {
char c = s.charAt(i);
switch (c) {
case '+':
buf[pos++] = ' '; // "+" -> " "
buf[pos++] = (byte) (DECODE_PLUS_AS_SPACE ? ' ' : '+'); // "+" -> " "
break;
case '%':
if (i == size - 1) {
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/rest/RestUtilsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public void testCrazyURL() {
randomDelimiter(),
randomDelimiter());
RestUtils.decodeQueryString(uri, uri.indexOf('?') + 1, params);
assertThat(params.get("/?:@-._~!$'()* ,"), equalTo("/?:@-._~!$'()* ,=="));
assertThat(params.get("/?:@-._~!$'()*+,"), equalTo("/?:@-._~!$'()*+,=="));
assertThat(params.size(), equalTo(1));
}

Expand Down
15 changes: 13 additions & 2 deletions ...y/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
import java.nio.charset.StandardCharsets;
import java.time.Clock;
import java.util.Base64;
Expand All @@ -19,6 +21,7 @@

import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.core.internal.io.Streams;
Expand Down Expand Up @@ -94,7 +97,7 @@ private ParsedQueryString parseQueryStringAndValidateSignature(String queryStrin
return new ParsedQueryString(samlRequest, true, relayState);
}

private Result parseLogout(LogoutRequest logoutRequest, boolean requireSignature, String relayState) {
private Result parseLogout(LogoutRequest logoutRequest, boolean requireSignature, @Nullable String relayState) {
final Signature signature = logoutRequest.getSignature();
if (signature == null) {
if (requireSignature) {
Expand All @@ -108,7 +111,15 @@ private Result parseLogout(LogoutRequest logoutRequest, boolean requireSignature
checkDestination(logoutRequest);
validateNotOnOrAfter(logoutRequest.getNotOnOrAfter());

return new Result(logoutRequest.getID(), SamlNameId.fromXml(getNameID(logoutRequest)), getSessionIndex(logoutRequest), relayState);
try {
return new Result(
logoutRequest.getID(), SamlNameId.fromXml(getNameID(logoutRequest)),
getSessionIndex(logoutRequest),
relayState == null ? null : URLDecoder.decode(relayState, StandardCharsets.US_ASCII.name())
Copy link
Member Author

Choose a reason for hiding this comment

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

This is kind of hacky and I wonder what the correct fix is here. The problem is that we use URLEncode.encode to encode this String but use our RestUtils to decode it when we parse the params from the URL.
This worked out symmetrically when we were handling the + sign as a space but won't be symmetric when we don't causing tests to fail that put a space in relayState.
=> makes me wonder if it's even correct to use our RestUtils parser here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I read some brief SO/docs on RelayState and it seems like we should not be doing decoding/encoding on it, but instead storing it as an "opaque object"... I think that it might be a correct assumption to not use the decodeQueryString here, but I dont know if we should take my word as gospel. Ill let @jasontedor comment further.

Copy link
Member

Choose a reason for hiding this comment

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

From what I have read, URL encoding is correct here, using RestUtils to decode looks wrong, and so the change here is valid. Let us validate this with @tvernum though.

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 is correct.
This string is already (mostly) decoded in parseQueryStringAndValidateSignature above, so any %-encoded entities will be expanded at that point.
If we decode here we will be double decoding.

Specifically the RelayState of 99%3a would be encoded on the wire as RelayState=99%253a.
Then parseQueryStringAndValidateSignature will parse that back to 99%3a
If we then try to UrlDecoder.decode that, it would end up at 99: which is incorrect.

I'll need to look into what we ought to be doing with +, but I don't think this change is correct.

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 @tvernum I think I figured this out actually and was able to revert this change.

The problem actually came from the assumption that we made here #33164 (comment) (never handling form data). It turns out that the form data encoding logic (+ -> ) applies to parameters in the URL. I now made the changes to the logic to decode + to a space in query params only and this test passes again without any changes to the code here :)

);
} catch (UnsupportedEncodingException e) {
throw new IllegalArgumentException(e);
}
}

private void validateSignature(String inputString, String signatureAlgorithm, String signature) {
Expand Down