mraible commented on code in PR #165:
URL: https://github.com/apache/roller/pull/165#discussion_r3891405457
##########
app/src/main/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServlet.java:
##########
@@ -71,40 +81,93 @@ public void doPost(HttpServletRequest request,
HttpServletResponse response)
try{
OAuthMessage requestMessage = OAuthServlet.getMessage(request,
null);
-
+
OAuthManager omgr =
WebloggerFactory.getWeblogger().getOAuthManager();
OAuthAccessor accessor = omgr.getAccessor(requestMessage);
-
- String userId = request.getParameter("userId");
- if (userId == null) {
- userId = request.getParameter("xoauth_requestor_id");
+ if (accessor == null || accessor.consumer == null ||
accessor.requestToken == null) {
+ denyPermission(response);
+ return;
}
-
- if (userId == null) {
- // no user associted with the key, must be site-wide key,
- // so get user to login and do the authorization process
+
+ // The approving identity comes from the browser session,
consistent
+ // with the rest of the UI. Without a session there is nobody to
+ // approve on behalf of, so send the caller through the login flow.
+ User user = getAuthenticatedUser(request);
+ if (user == null) {
sendToAuthorizePage(request, response, accessor);
-
- } else {
+ return;
+ }
+ if (!Boolean.TRUE.equals(user.getEnabled())) {
+ denyPermission(response);
+ return;
+ }
+ String userId = user.getUserName();
- // if consumer key is for specific user, check username match
- String consumerUserId =
(String)accessor.consumer.getProperty("userId");
- if (consumerUserId != null && !userId.equals(consumerUserId)) {
- throw new ServletException("ERROR: invalid or unspecified
userId");
- }
+ // A consumer key bound to one user may only be approved by that
+ // user. A site-wide key has no bound user and is approved as
+ // whoever is logged in.
+ String consumerUserId =
(String)accessor.consumer.getProperty("userId");
+ if (consumerUserId != null && !consumerUserId.equals(userId)) {
+ denyPermission(response);
+ return;
+ }
- // set userId in accessor and mark it as authorized
- omgr.markAsAuthorized(accessor, userId);
- WebloggerFactory.getWeblogger().flush();
+ // Older clients still post the identity; accept it only when it
+ // agrees with the session.
+ String submittedUserId = request.getParameter("userId");
+ if (submittedUserId == null) {
+ submittedUserId = request.getParameter("xoauth_requestor_id");
}
-
+ if (submittedUserId != null && !submittedUserId.equals(userId)) {
+ denyPermission(response);
+ return;
+ }
+
+ // Claim the pending request token in one conditional statement, so
+ // approval is one-shot. A token that is missing, belongs to
another
+ // consumer, or has already been approved or exchanged all produce
+ // the same answer here and the same response below.
+ if (!omgr.authorizeRequestToken(
+ accessor.consumer.consumerKey, accessor.requestToken,
userId)) {
+ denyPermission(response);
+ return;
+ }
+ WebloggerFactory.getWeblogger().flush();
+
+ accessor.setProperty("userId", userId);
+ accessor.setProperty("authorized", Boolean.TRUE);
+
returnToConsumer(request, response, accessor);
Review Comment:
This is where the fixation attack survives. Attacker starts the consumer
flow, gets request token T, and sends a logged-in victim
`/roller-services/oauth/authorize?oauth_token=T`. The victim clicks Authorize,
this conditional update binds T to the victim, and the attacker finishes the
exchange at the consumer: `AccessTokenServlet` checks only `authorized ==
TRUE`, so it hands out an access token acting as the victim. The session check
and one-shot approval don't help because the attacker never posts here. OAuth
1.0a fixed exactly this with `oauth_verifier`: generate one on approval, store
it on the accessor, return it to the consumer through the callback, and require
it on the access-token exchange.
##########
app/src/main/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServlet.java:
##########
@@ -71,40 +81,93 @@ public void doPost(HttpServletRequest request,
HttpServletResponse response)
try{
OAuthMessage requestMessage = OAuthServlet.getMessage(request,
null);
-
+
OAuthManager omgr =
WebloggerFactory.getWeblogger().getOAuthManager();
OAuthAccessor accessor = omgr.getAccessor(requestMessage);
-
- String userId = request.getParameter("userId");
- if (userId == null) {
- userId = request.getParameter("xoauth_requestor_id");
+ if (accessor == null || accessor.consumer == null ||
accessor.requestToken == null) {
+ denyPermission(response);
+ return;
}
-
- if (userId == null) {
- // no user associted with the key, must be site-wide key,
- // so get user to login and do the authorization process
+
+ // The approving identity comes from the browser session,
consistent
+ // with the rest of the UI. Without a session there is nobody to
+ // approve on behalf of, so send the caller through the login flow.
+ User user = getAuthenticatedUser(request);
+ if (user == null) {
sendToAuthorizePage(request, response, accessor);
-
- } else {
+ return;
+ }
+ if (!Boolean.TRUE.equals(user.getEnabled())) {
+ denyPermission(response);
+ return;
+ }
+ String userId = user.getUserName();
- // if consumer key is for specific user, check username match
- String consumerUserId =
(String)accessor.consumer.getProperty("userId");
- if (consumerUserId != null && !userId.equals(consumerUserId)) {
- throw new ServletException("ERROR: invalid or unspecified
userId");
- }
+ // A consumer key bound to one user may only be approved by that
+ // user. A site-wide key has no bound user and is approved as
+ // whoever is logged in.
+ String consumerUserId =
(String)accessor.consumer.getProperty("userId");
+ if (consumerUserId != null && !consumerUserId.equals(userId)) {
+ denyPermission(response);
+ return;
+ }
- // set userId in accessor and mark it as authorized
- omgr.markAsAuthorized(accessor, userId);
- WebloggerFactory.getWeblogger().flush();
+ // Older clients still post the identity; accept it only when it
+ // agrees with the session.
+ String submittedUserId = request.getParameter("userId");
+ if (submittedUserId == null) {
+ submittedUserId = request.getParameter("xoauth_requestor_id");
}
-
+ if (submittedUserId != null && !submittedUserId.equals(userId)) {
+ denyPermission(response);
+ return;
+ }
+
+ // Claim the pending request token in one conditional statement, so
+ // approval is one-shot. A token that is missing, belongs to
another
+ // consumer, or has already been approved or exchanged all produce
+ // the same answer here and the same response below.
+ if (!omgr.authorizeRequestToken(
+ accessor.consumer.consumerKey, accessor.requestToken,
userId)) {
+ denyPermission(response);
+ return;
+ }
+ WebloggerFactory.getWeblogger().flush();
+
+ accessor.setProperty("userId", userId);
+ accessor.setProperty("authorized", Boolean.TRUE);
+
returnToConsumer(request, response, accessor);
-
+
+ } catch (OAuthProblemException e) {
+ denyPermission(response);
} catch (Exception e){
handleException(e, request, response, true);
}
}
-
+
+ /**
+ * The Roller user behind this request's session, or null if there is none.
+ */
+ private User getAuthenticatedUser(HttpServletRequest request) {
+ RollerSession rollerSession = RollerSession.getRollerSession(request);
+ return rollerSession == null ? null :
rollerSession.getAuthenticatedUser();
+ }
+
+ /**
+ * Refuse the approval, in the OAuth problem-reporting form and with the
+ * same body for every reason. Written directly rather than thrown so the
+ * response does not vary with how the library happens to render a given
+ * exception.
+ */
+ private void denyPermission(HttpServletResponse response) throws
IOException {
+ response.setStatus(HttpServletResponse.SC_FORBIDDEN);
+ response.setContentType("text/plain");
+ try (PrintWriter out = response.getWriter()) {
+ out.println("oauth_problem=" + PERMISSION_DENIED);
+ }
+ }
+
private void sendToAuthorizePage(HttpServletRequest request,
HttpServletResponse response, OAuthAccessor accessor)
throws IOException, ServletException{
Review Comment:
`returnToConsumer` (line 190) redirects to the `oauth_callback` request
parameter, which is attacker-controllable and seeded into the consent form's
hidden field from the original GET link. A victim sent
...authorize?`oauth_token`=T&`oauth_callback`=`https://evil.example/` gets
302'd there with the freshly authorized token on the query string, and for an
already-authorized T the GET path redirects with no click at all. Pre-existing,
but this PR reworks the success path, so it's the moment to validate the
callback against the consumer's registered one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
