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.
##########
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/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.
##########
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/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:
##########
@@ -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.
--
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]
