Disable vendor extension types on the XML-RPC servlet - #171
Conversation
Roller's XML-RPC API uses only the standard XML-RPC value types; the library's vendor extension types are unused and are switched off. The endpoint is also gated on webservices.enableXmlRpc so a disabled service is not reachable and does not read request bodies. Adds XmlRpcExtensionTypeTest, which checks the shipped configuration and that ordinary calls still parse. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV
mraible
left a comment
mraible
left a comment
There was a problem hiding this comment.
The change itself is small and right: enabledForExtensions=false closes the ex:serializable deserialisation path (CVE-2019-17570) on the servlet, and a filter is a sane place for the runtime toggle. Approving, with a few things I'd still like addressed inline and two notes for the description:
- Turning off extensions also turns off the library's gzip request/response handling (
XmlRpcStreamServer.getInputStream/getOutputStreamboth checkisEnabledForExtensions). Clients using Apache XML-RPC withsetGzipCompressing(true)will start getting parse faults. That's by design of the library (gzip is an extension), but it's a behaviour change worth stating. - This only covers the inbound servlet.
WeblogUpdatePingerstill usesXmlRpcClient3.1.3, whoseXmlRpcResponseParserdeserialises a base64faultCausefrom any fault response regardless of this flag, so a malicious or hijacked ping target can still get code execution. Out of scope here, but it's the same CVE and deserves its own PR.
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug("XML-RPC service is disabled; rejecting request"); | ||
| } | ||
| ((HttpServletResponse) response).sendError(HttpServletResponse.SC_NOT_FOUND); |
There was a problem hiding this comment.
sendError(404) gets routed to /roller-ui/errors/404.jsp, so a disabled endpoint now answers a desktop client's POST with an HTML page instead of the XML-RPC fault BLOGGERAPI_DISABLED_MSG that BaseAPIHandler still produces (line 152) but can no longer reach. Clients like MarsEdit will show a generic "invalid response". Consider writing a small XML-RPC fault with text/xml here, or at least setStatus(404) with a plain-text body so the error JSP isn't rendered.
| public void doFilter(ServletRequest request, ServletResponse response, | ||
| FilterChain chain) throws IOException, ServletException { | ||
|
|
||
| if (!WebloggerRuntimeConfig.getBooleanProperty("webservices.enableXmlRpc")) { |
There was a problem hiding this comment.
getBooleanProperty swallows exceptions and returns false, so a DB hiccup or a request before bootstrap answers 404 rather than an error. A LOG.warn when refusing would make that diagnosable; debug is easy to miss.
| </filter-mapping> | ||
|
|
||
| <!-- Request mapping, this is what allows the urls to work --> | ||
| <filter-mapping> |
There was a problem hiding this comment.
Nit: no <dispatcher> so this applies to REQUEST only; nothing forwards to /roller-services/xmlrpc today, but adding FORWARD / INCLUDE makes the gate match what the javadoc claims.
| "the gating filter must be mapped to the XML-RPC endpoint"); | ||
| } | ||
|
|
||
| /** Ordinary XML-RPC calls must still parse with extensions disabled. */ |
There was a problem hiding this comment.
The description says extension types are rejected and the filter is tested, but this class only parses an ordinary call. A request containing <ex:serializable> (or <ex:nil/>) asserted to fail with extensions disabled, plus an XmlRpcEnabledFilter test with a mocked sendError, would actually pin the security property this PR introduces.
| "a filter must gate the XML-RPC endpoint"); | ||
| int mapping = webXml.indexOf("<filter-name>XmlRpcEnabledFilter</filter-name>", | ||
| webXml.indexOf("<filter-mapping>")); | ||
| assertTrue(mapping > 0, "the gating filter must be mapped"); |
There was a problem hiding this comment.
indexOf("/roller-services/xmlrpc", mapping) > 0 is also satisfied by the later <servlet-mapping> for XmlRpcServlet, so a typo in the filter's url-pattern still passes. Parse the <filter-mapping> element and compare its url-pattern directly.
| public class XmlRpcExtensionTypeTest { | ||
|
|
||
| private static final Path WEB_XML = | ||
| Paths.get("src", "main", "webapp", "WEB-INF", "web.xml"); |
There was a problem hiding this comment.
cwd-relative; surefire sets project.build.directory for this module and ApplicationResourcesTest / SQLScriptRunnerTest derive their paths from it.
| Paths.get("src", "main", "webapp", "WEB-INF", "web.xml"); | ||
|
|
||
| private String parseRequest(String xml, boolean extensionsEnabled) throws Exception { | ||
| XmlRpcServer server = new XmlRpcServer(); |
There was a problem hiding this comment.
Nit: extensionsEnabled is only ever false, the (XmlRpcController) cast is a no-op, and XMLReaderFactory has been deprecated since Java 9 (SAXParserFactory.newInstance().newSAXParser().getXMLReader()).
2 participants
The XML-RPC servlet is configured to accept the library's non-standard vendor
extension types, which Roller does not use, and its mapped endpoint answers
requests even when the XML-RPC feature is switched off. This change turns the
extensions off and makes the disabled-by-default toggle close the endpoint.
What changed
enabledForExtensions=false, or drop theenabling servlet init-param), since Roller uses only the standard XML-RPC
value types.
webservices.enableXmlRpc, so a disabled API isclosed at the endpoint instead of relying on per-handler checks.
Tests
the endpoint.
XML-RPC call reaches its handler.