HDDS-2158. Fixing Json Injection Issue in JsonUtils. #1486
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| System.out.printf("%s%n", JsonUtils.toJsonStringWithDefaultPrettyPrinter( | ||
| JsonUtils.toJsonString("Acl set successfully: " + result))); | ||
| System.out.printf("%s%n", "Acl set successfully: " + | ||
| JsonUtils.toJsonStringWithDefaultPrettyPrinter(result)); |
There was a problem hiding this comment.
Here the result is true/false, we can directly print. Do we need toJsonStringWithDefaultPrettyPrinter here? Previously this was called with Acl set successfully: + result. But now just result, so is it okay if we directly use result to print?
Same comment for all AclHandler classes.
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
I think this should be fine, as we are not converting to String and printing. (Same for others too)
There was a problem hiding this comment.
Isn't that what toJsonStringWithDefaultPrettyPrinter does - i.e. convert to string. Then we print it out.
There was a problem hiding this comment.
Yes, toJsonStringWithDefaultPrettyPrinter converts to string but in Json format (with proper indentations and new line for each element).
Previously, we were converting the object to be printed to string (using JsonUtils.toJsonString(result)), then converting it back into object (inside old toJsonStringWithDefaultPrettyPrinter()) and then again converting it into string.
//Old code
public static String toJsonStringWithDefaultPrettyPrinter(String jsonString)
throws IOException {
Object json = READER.readValue(jsonString);
return WRITTER.writeValueAsString(json);
}
The Json Injection issue happens when converting the jsonString back to Object without validation.
...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/web/ozShell/token/GetTokenHandler.java
Outdated
Show resolved
Hide resolved
...ozone-manager/src/main/java/org/apache/hadoop/ozone/web/ozShell/token/PrintTokenHandler.java
Outdated
Show resolved
Hide resolved
...ne-manager/src/main/java/org/apache/hadoop/ozone/web/ozShell/volume/GetAclVolumeHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
False means Acl passed is already existing, in this case we return false. So message ACL not set is meaningful here or do we need to reword this?
There was a problem hiding this comment.
RemoveAcl returns false, when passedAcl is not existing, then it cannot be removed. Do we need to reword this to convey this?
There was a problem hiding this comment.
I reverted to previous print messages (fixing add/set/remove part only).
But I think that statement also does not convey the message properly. If we are trying to add an already existing ACL, shouldn't we return true? And same for remove also.
We can open a new Jira to work on that if you agree.
There was a problem hiding this comment.
From my understanding, addAcl behavior is if acl is added successfully returns true, it will return false when acl trying to be added already exists.
If we are trying to add an already existing ACL, shouldn't we return true?
I think returning true is not right behavior, as it will not be clear whether add is successful or not. We should have returned with clear message to end user, what is differenece between true/false.
But I think that statement also does not convey the message properly.
Agreed this was existing behavior, if you want to fix in a new Jira I am okay with that.
hanishakoneru
force-pushed
the
HDDS-2158
branch
from
a0dbd14 to
74a5248
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Thanks @bharatviswa504 and @arp7 for the reviews. |
|
Remaining failures are not related to this patch. |
4 participants
JsonUtils#toJsonStringWithDefaultPrettyPrinter() does not validate the Json String before serializing it which could result in Json Injection.
This method is mostly used along with JsonUtils#toJsonString() by first converting to String and then back to object. This can be avoided by directly writing the object through PrettyPrinter.