mraible commented on code in PR #165:
URL: https://github.com/apache/roller/pull/165#discussion_r3891405462
##########
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);
Review Comment:
The comment says a session-less request goes through the login flow, but
sendToAuthorizePage forwards to /roller-ui/oauthAuthorize.rol, which isn't a
Spring-protected URL: UISecurityInterceptor returns DENIED and the user lands
on the access-denied tile with no way to log in or resume. A user whose login
expired mid-consent, or a first-time consent with no session, dead-ends there
and the consumer's request token is stranded. A redirect to the login page with
a saved request would do what the comment describes.
##########
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.
##########
app/src/main/webapp/WEB-INF/web.xml:
##########
@@ -153,6 +153,16 @@
<url-pattern>/roller-ui/*</url-pattern>
</filter-mapping>
+ <!-- The OAuth consent form is served by a forward into /roller-ui/, so
it
+ already receives a token from LoadSaltFilter above, but it posts
back
+ outside /roller-ui/. Validate that one exact URL; the
request-token and
+ access-token endpoints are consumer-to-server calls carrying an
OAuth
+ signature and must not be included. -->
+ <filter-mapping>
+ <filter-name>ValidateSaltFilter</filter-name>
+ <url-pattern>/roller-services/oauth/authorize</url-pattern>
Review Comment:
Good to gate the consent POST, but ValidateSaltFilter throws
ServletException("Security Violation") when the salt is missing, expired
(SaltCache entries live 60 minutes, and cache.salt.size=5000 evicts), already
consumed, or issued to a different user. A user who leaves the consent page
open for an hour and then clicks Authorize gets the container's 500 page rather
than the consent form again or an OAuth problem response. Same on a cluster
without a shared SaltCache. Catching that case in the servlet (re-render with a
fresh salt) would keep the protection without the cliff.
##########
app/src/main/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServlet.java:
##########
@@ -42,7 +45,14 @@
*/
public class AuthorizationServlet extends HttpServlet {
protected static final Log log =
LogFactory.getFactory().getInstance(AuthorizationServlet.class);
-
+
+ /**
+ * One response for every refusal, so the endpoint reveals nothing about
+ * tokens the caller does not hold.
+ */
+ private static final String PERMISSION_DENIED = "permission_denied";
+
+
@Override
public void doGet(HttpServletRequest request, HttpServletResponse response)
throws IOException, ServletException {
Review Comment:
doGet still dereferences accessor without the null guard doPost gained (line
66): GET ...authorize?oauth_consumer_key=<key> before any request token exists
NPEs on accessor.getProperty and surfaces as a container 500, where doPost now
returns the uniform permission_denied.
##########
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) {
Review Comment:
Previously OAuthProblemException from getAccessor (token_expired,
token_rejected) went through OAuthServlet.handleException, which sends the
problem-specific status and a WWW-Authenticate: OAuth realm header. This catch
collapses everything to a bare 403 with no realm header, while doGet on the
same endpoint still reports the old way, so the same token state is described
two different ways depending on method.
##########
app/src/main/java/org/apache/roller/weblogger/business/OAuthManager.java:
##########
@@ -88,10 +88,39 @@ OAuthAccessor getAccessor(OAuthMessage requestMessage)
throws IOException, OAuthProblemException;
/**
- * Set the access token
+ * Set the access token
+ *
+ * @deprecated Records approval against the consumer key alone, without
+ * naming the request token being approved and without
+ * requiring that it is still pending. Use
+ * {@link #authorizeRequestToken(String, String, String)},
+ * which does both in one statement. No longer called from
+ * Roller; retained for callers outside the project.
*/
+ @Deprecated
void markAsAuthorized(OAuthAccessor accessor, String userId)
- throws OAuthException;
+ throws OAuthException;
+
+ /**
+ * Record a user's approval of one pending request token.
+ *
+ * <p>The whole transition happens in a single conditional statement: the
+ * record is claimed only if it still matches the consumer key and the
+ * exact request token, has not been authorized already, and has not yet
+ * been exchanged for an access token. That makes approval one-shot without
+ * a read-then-write window in which the same token could be approved
+ * twice.
+ *
+ * @param consumerKey key of the consumer the token was issued to
+ * @param requestToken the pending request token being approved
+ * @param userName the approving user
+ * @return true if this call performed the approval; false if the record
+ * did not match, was already authorized, or was already exchanged.
+ * Callers should not distinguish these cases to the client.
+ * @throws OAuthException on persistence failure
+ */
+ boolean authorizeRequestToken(String consumerKey, String requestToken,
String userName)
Review Comment:
Minor: adding an abstract method to this interface while keeping
markAsAuthorized deprecated 'for callers outside the project' is a bit
contradictory; if external implementations are a concern, a default method
covers them, and if they aren't, markAsAuthorized can just go.
##########
app/src/test/java/org/apache/roller/weblogger/webservices/oauth/AuthorizationServletTest.java:
##########
@@ -0,0 +1,325 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. The ASF licenses this file to You
+* under the Apache License, Version 2.0 (the "License"); you may not
+* use this file except in compliance with the License.
+* You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License. For additional information regarding
+* copyright in this work, please see the NOTICE file in the top level
+* directory of this distribution.
+*/
+
+package org.apache.roller.weblogger.webservices.oauth;
+
+import net.oauth.OAuthAccessor;
+import net.oauth.OAuthConsumer;
+import net.oauth.OAuthProblemException;
+import org.apache.roller.weblogger.business.OAuthManager;
+import org.apache.roller.weblogger.business.Weblogger;
+import org.apache.roller.weblogger.business.WebloggerFactory;
+import org.apache.roller.weblogger.pojos.User;
+import org.apache.roller.weblogger.ui.core.RollerSession;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.MockitoAnnotations;
+
+import javax.servlet.RequestDispatcher;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.*;
+
+/**
+ * Verifies which identity {@link AuthorizationServlet} authorizes a request
+ * token for.
+ *
+ * <p>The servlet runs the browser consent step: a logged-in user approves a
+ * consumer's pending request token. The identity being approved is a property
+ * of the session, so these tests pin it there and check that values arriving
+ * in the request body cannot redirect the approval onto a different account.
+ */
+public class AuthorizationServletTest {
+
+ private static final String CONSUMER_KEY = "test-consumer-key";
+ private static final String REQUEST_TOKEN = "test-request-token";
+
+ private AuthorizationServlet servlet;
+
+ @Mock
+ private HttpServletRequest request;
+
+ @Mock
+ private HttpServletResponse response;
+
+ @Mock
+ private RequestDispatcher dispatcher;
+
+ @Mock
+ private RollerSession rollerSession;
+
+ @Mock
+ private Weblogger weblogger;
+
+ @Mock
+ private OAuthManager oauthManager;
+
+ private OAuthAccessor accessor;
+ private StringWriter responseBody;
+
+ @BeforeEach
+ public void setUp() throws Exception {
+ MockitoAnnotations.openMocks(this);
+ servlet = new AuthorizationServlet();
+
+ // A site-wide consumer: no "userId" property bound to the key. This is
+ // the configuration in which the servlet has no consumer-side identity
+ // to compare against and must fall back on the session.
+ OAuthConsumer consumer =
+ new OAuthConsumer("http://example.com/callback";, CONSUMER_KEY,
"secret", null);
+ accessor = new OAuthAccessor(consumer);
+ accessor.requestToken = REQUEST_TOKEN;
+
+ when(request.getMethod()).thenReturn("POST");
+ when(request.getRequestURL())
+ .thenReturn(new
StringBuffer("https://example.com/roller-services/oauth/authorize";));
+ when(request.getLocalName()).thenReturn("example.com");
+ when(request.getRequestDispatcher(anyString())).thenReturn(dispatcher);
+ responseBody = new StringWriter();
+ when(response.getWriter()).thenReturn(new PrintWriter(responseBody));
+
+ when(weblogger.getOAuthManager()).thenReturn(oauthManager);
+ when(oauthManager.getAccessor(any())).thenReturn(accessor);
+ }
+
+ private User user(String userName) {
+ User u = new User();
+ u.setUserName(userName);
+ u.setEnabled(Boolean.TRUE);
+ return u;
+ }
+
+ /**
+ * Assert that no approval was recorded for {@code userName}, through
either
+ * of the manager's authorizing entry points. Checking both matters: a test
+ * that named only one of them would pass whenever the servlet happened to
+ * use the other.
+ */
+ private void verifyNothingAuthorizedFor(String userName) throws Exception {
+ verify(oauthManager, never()).markAsAuthorized(any(), eq(userName));
+ verify(oauthManager, never()).authorizeRequestToken(anyString(),
anyString(), eq(userName));
+ }
+
+ /**
+ * Assert that no approval was recorded at all.
+ */
+ private void verifyNothingAuthorized() throws Exception {
+ verify(oauthManager, never()).markAsAuthorized(any(), anyString());
+ verify(oauthManager, never()).authorizeRequestToken(anyString(),
anyString(), anyString());
+ }
+
+ /**
+ * The session belongs to "alice" but the posted form names "admin". The
+ * approval must not be recorded for "admin".
+ */
+ @Test
+ public void postedUserIdDoesNotChooseTheIdentity() throws Exception {
+ try (MockedStatic<WebloggerFactory> factory =
mockStatic(WebloggerFactory.class);
+ MockedStatic<RollerSession> session =
mockStatic(RollerSession.class)) {
+
+ factory.when(WebloggerFactory::getWeblogger).thenReturn(weblogger);
+ session.when(() ->
RollerSession.getRollerSession(request)).thenReturn(rollerSession);
+
when(rollerSession.getAuthenticatedUser()).thenReturn(user("alice"));
+
+ when(request.getParameter("userId")).thenReturn("admin");
+
+ servlet.doPost(request, response);
+
+ verifyNothingAuthorizedFor("admin");
Review Comment:
The mismatched-identity tests assert that nothing was authorized for admin,
but not the 403, and not that alice wasn't authorized either. As written they'd
pass if the mismatch were silently accepted for alice. Asserting the response
status and the absence of any authorization pins the behavior the description
promises.
##########
app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAOAuthManagerImpl.java:
##########
@@ -139,6 +140,25 @@ public void markAsAuthorized(OAuthAccessor accessor,
String userId)
}
}
+ @Override
+ public boolean authorizeRequestToken(String consumerKey, String
requestToken, String userName)
+ throws OAuthException {
+ if (consumerKey == null || requestToken == null || userName == null) {
+ return false;
+ }
+ try {
+ Query q =
strategy.getNamedUpdate("OAuthAccessorRecord.authorizeRequestToken");
+ q.setParameter(1, userName);
+ q.setParameter(2, new Timestamp(new Date().getTime()));
+ q.setParameter(3, consumerKey);
+ q.setParameter(4, requestToken);
+ return q.executeUpdate() == 1;
Review Comment:
markAsAuthorized was idempotent; this update requires authorized to be null
or false, so approving a token that's already authorized but not yet exchanged
affects zero rows and the servlet answers a bare 403 permission_denied instead
of returning the user to the consumer. That happens on a retried or
double-submitted approval. Treating already-authorized-by-the-same-user as
success keeps the one-shot guarantee without breaking retries.
--
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]
