diff --git a/Modules/Sources/WordPressData/Swift/Blog+InsecureXMLRPCEndpoint.swift b/Modules/Sources/WordPressData/Swift/Blog+InsecureXMLRPCEndpoint.swift new file mode 100644 index 000000000000..ef65c052a0bf --- /dev/null +++ b/Modules/Sources/WordPressData/Swift/Blog+InsecureXMLRPCEndpoint.swift @@ -0,0 +1,33 @@ +import Foundation + +extension Blog { + + /// The XML-RPC endpoint URL to use for network requests. + /// + /// Older app versions could silently downgrade the discovered XML-RPC endpoint to + /// http for an https site and persist it, so later XML-RPC traffic and the + /// credentials it carries crossed plaintext (GHSA-qxpr-7v78-mh5g). For such a site + /// this returns the https-upgraded endpoint, so a client built from it starts every + /// request over https. The persisted `xmlrpc` value and the Keychain entry keyed by + /// it are left untouched (so credential lookups still resolve); only the request + /// endpoint is upgraded, at the point a client is built. Every credential-bearing + /// XML-RPC client must be constructed from this, not from the raw `xmlrpc` string. + /// + /// This upgrades only the initial endpoint. It does not stop an https-to-http + /// redirect issued by the server during a request; blocking that in the XML-RPC + /// client is tracked separately. + @objc public var xmlrpcURL: URL? { + guard let xmlrpc, let endpoint = URL(string: xmlrpc) else { return nil } + // URL schemes are case-insensitive, so compare normalized schemes and rewrite + // through URL components: an http endpoint (in any spelling) for an https site + // is upgraded to https; everything else is returned unchanged. + guard endpoint.scheme?.lowercased() == "http", + let siteAddress = url, URL(string: siteAddress)?.scheme?.lowercased() == "https", + var components = URLComponents(url: endpoint, resolvingAgainstBaseURL: false) + else { + return endpoint + } + components.scheme = "https" + return components.url ?? endpoint + } +} diff --git a/Modules/Sources/WordPressData/Swift/Blog.swift b/Modules/Sources/WordPressData/Swift/Blog.swift index cdb49be9f4c9..8898126a69fd 100644 --- a/Modules/Sources/WordPressData/Swift/Blog.swift +++ b/Modules/Sources/WordPressData/Swift/Blog.swift @@ -68,17 +68,25 @@ public class Blog: NSManagedObject { // MARK: - Non-Core Data Properties private var _xmlrpcApi: WordPressOrgXMLRPCApi? + private var _xmlrpcApiEndpoint: URL? private var _selfHostedSiteRestApi: WordPressOrgRestApi? @objc public var xmlrpcApi: WordPressOrgXMLRPCApi? { get { - if _xmlrpcApi == nil, let endpoint = xmlrpc.flatMap(URL.init(string:)) { - _xmlrpcApi = WordPressOrgXMLRPCApi(endpoint: endpoint, userAgent: WPUserAgent.wordPress()) + let endpoint = xmlrpcURL + // The endpoint depends on both `xmlrpc` and `url` (via `xmlrpcURL`), and + // `url` can change without clearing this cache (including through a merge + // from another Core Data context). Rebuild whenever the computed endpoint + // changes so a cached client never keeps sending to a stale endpoint. + if _xmlrpcApiEndpoint != endpoint { + _xmlrpcApi = endpoint.map { WordPressOrgXMLRPCApi(endpoint: $0, userAgent: WPUserAgent.wordPress()) } + _xmlrpcApiEndpoint = endpoint } return _xmlrpcApi } set { _xmlrpcApi = newValue + _xmlrpcApiEndpoint = newValue == nil ? nil : xmlrpcURL } } diff --git a/Modules/Sources/WordPressKit/WordPressOrgXMLRPCValidator.swift b/Modules/Sources/WordPressKit/WordPressOrgXMLRPCValidator.swift index 05360e912cd7..389c7b5c897f 100644 --- a/Modules/Sources/WordPressKit/WordPressOrgXMLRPCValidator.swift +++ b/Modules/Sources/WordPressKit/WordPressOrgXMLRPCValidator.swift @@ -10,6 +10,7 @@ import Foundation case blocked = 405 // Server returned a 405 error while reading xmlrpc file case invalid // Doesn't look to be valid XMLRPC Endpoint. case xmlrpc_missing // site contains RSD link but XML-RPC information is missing + case insecureEndpoint // The endpoint resolved to plain http for a site that was requested over https public var localizedDescription: String { switch self { @@ -34,6 +35,11 @@ import Foundation return NSLocalizedString("Couldn't connect. We received a 403 error when trying to access your site's XMLRPC endpoint. The app needs that in order to communicate with your site. Please contact your hosting provider to solve this problem.", comment: "Message to show to user when he tries to add a self-hosted site but the host returned a 403 error, meaning that the access to the /xmlrpc.php file is forbidden.") case .xmlrpc_missing: return NSLocalizedString("Couldn't connect. Required XML-RPC methods are missing on the server. Please contact your hosting provider to solve this problem.", comment: "Message to show to user when he tries to add a self-hosted site with RSD link present, but xmlrpc is missing.") + case .insecureEndpoint: + return NSLocalizedString( + "Couldn't establish a secure connection to your site's XML-RPC endpoint.", + comment: "Error message shown when the discovered XML-RPC endpoint is not served over HTTPS even though the site address uses HTTPS." + ) } } } @@ -95,15 +101,28 @@ open class WordPressOrgXMLRPCValidator: NSObject { sitesToTry.append(site.replacingOccurrences(of: "http://", with: "https://")) } else if site.hasPrefix("https://") { sitesToTry.append(site) - if !secureAccessOnly { - sitesToTry.append(site.replacingOccurrences(of: "https://", with: "http://")) - } } else { failure(WordPressOrgXMLRPCValidatorError.invalidScheme as NSError) return } - tryGuessXMLRPCURLForSites(sitesToTry, userAgent: userAgent, success: success, failure: failure) + // Never hand back a plaintext endpoint when the caller asked for a secure site. + // Besides the (removed) http probe candidate, redirects and RSD discovery can + // also resolve to http:// for an https:// input (GHSA-qxpr-7v78-mh5g). + let validatedSuccess: (URL) -> Void + if site.hasPrefix("https://") { + validatedSuccess = { xmlrpcURL in + if xmlrpcURL.scheme?.lowercased() == "https" { + success(xmlrpcURL) + } else { + failure(WordPressOrgXMLRPCValidatorError.insecureEndpoint as NSError) + } + } + } else { + validatedSuccess = success + } + + tryGuessXMLRPCURLForSites(sitesToTry, userAgent: userAgent, success: validatedSuccess, failure: failure) } /// Helper for `guessXMLRPCURLForSite(_:userAgent:success:failure)` diff --git a/Modules/Tests/WordPressDataTests/BlogInsecureXMLRPCEndpointTests.swift b/Modules/Tests/WordPressDataTests/BlogInsecureXMLRPCEndpointTests.swift new file mode 100644 index 000000000000..99fbe98851b0 --- /dev/null +++ b/Modules/Tests/WordPressDataTests/BlogInsecureXMLRPCEndpointTests.swift @@ -0,0 +1,70 @@ +import CoreData +import Testing +@testable import WordPressData + +@MainActor +struct BlogInsecureXMLRPCEndpointTests { + private let contextManager = ContextManager.forTesting() + + private func makeBlog(url: String?, xmlrpc: String?) -> Blog { + let blog = BlogBuilder(contextManager.mainContext, dotComID: nil).build() + blog.account = nil + blog.url = url + blog.xmlrpc = xmlrpc + return blog + } + + @Test func upgradesHTTPEndpointForHTTPSSite() { + let blog = makeBlog(url: "https://example.com", xmlrpc: "http://example.com/xmlrpc.php") + #expect(blog.xmlrpcURL?.absoluteString == "https://example.com/xmlrpc.php") + } + + @Test func leavesSecureEndpointUnchanged() { + let blog = makeBlog(url: "https://example.com", xmlrpc: "https://example.com/xmlrpc.php") + #expect(blog.xmlrpcURL?.absoluteString == "https://example.com/xmlrpc.php") + } + + @Test func leavesHTTPSiteEndpointUnchanged() { + // The site itself is http (user-asserted), so the endpoint is left as-is. + let blog = makeBlog(url: "http://example.com", xmlrpc: "http://example.com/xmlrpc.php") + #expect(blog.xmlrpcURL?.absoluteString == "http://example.com/xmlrpc.php") + } + + @Test func returnsNilWhenNoEndpoint() { + let blog = makeBlog(url: "https://example.com", xmlrpc: nil) + #expect(blog.xmlrpcURL == nil) + } + + @Test func upgradesMixedCaseHTTPEndpoint() { + // URL schemes are case-insensitive, so an uppercase http scheme must still upgrade. + let blog = makeBlog(url: "https://example.com", xmlrpc: "HTTP://example.com/xmlrpc.php") + #expect(blog.xmlrpcURL?.scheme == "https") + #expect(blog.xmlrpcURL?.host == "example.com") + #expect(blog.xmlrpcURL?.path == "/xmlrpc.php") + } + + @Test func upgradesForMixedCaseHTTPSSite() { + let blog = makeBlog(url: "HTTPS://example.com", xmlrpc: "http://example.com/xmlrpc.php") + #expect(blog.xmlrpcURL?.absoluteString == "https://example.com/xmlrpc.php") + } + + @Test func rebuildsCachedClientWhenEndpointChanges() throws { + // Realize the client while the site is http (no upgrade), then flip the site + // address to https. The cached client must be rebuilt for the upgraded endpoint + // rather than keep sending to the stale http one. + let blog = makeBlog(url: "http://example.com", xmlrpc: "http://example.com/xmlrpc.php") + let httpClient = try #require(blog.xmlrpcApi) + + blog.url = "https://example.com" + + let upgradedClient = try #require(blog.xmlrpcApi) + #expect(upgradedClient !== httpClient) + } + + @Test func reusesCachedClientWhenEndpointUnchanged() throws { + let blog = makeBlog(url: "https://example.com", xmlrpc: "http://example.com/xmlrpc.php") + let first = try #require(blog.xmlrpcApi) + let second = try #require(blog.xmlrpcApi) + #expect(first === second) + } +} diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index f5ebeb9a62db..a61c3554e670 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -1,7 +1,7 @@ 27.2 ----- * [*] Stop the media picker from removing gallery images when you cancel it in the experimental editor [#25866] - +* [*] Fix an issue where XML-RPC credentials for an https self-hosted site could be sent over insecure http [#25869] 27.1 ----- diff --git a/Tests/WordPressKitTests/CoreAPITests/WordPressOrgXMLRPCValidatorTests.swift b/Tests/WordPressKitTests/CoreAPITests/WordPressOrgXMLRPCValidatorTests.swift index a66c00d7abd1..ba484b7f5208 100644 --- a/Tests/WordPressKitTests/CoreAPITests/WordPressOrgXMLRPCValidatorTests.swift +++ b/Tests/WordPressKitTests/CoreAPITests/WordPressOrgXMLRPCValidatorTests.swift @@ -368,6 +368,83 @@ final class WordPressOrgXMLRPCValidatorTests: XCTestCase { wait(for: [failure], timeout: 0.3) } + func testItWillNotGuessXMLRPCOnHTTPForHTTPSInput() { + // Given + var schemes = Set() + // Stub all, we only care about the URL schemes that are being tested. + stub(condition: { request -> Bool in + if let scheme = request.url?.scheme { + schemes.insert(scheme) + } + return true + }, response: { _ in + let error = NSError(domain: NSURLErrorDomain, code: NSURLErrorNotConnectedToInternet, userInfo: nil) + return HTTPStubsResponse(error: error) + }) + + // Unsecured ATS settings armed the http fallback before the fix (GHSA-qxpr-7v78-mh5g). + let validator = WordPressOrgXMLRPCValidator(makeUnsecuredAppTransportSecuritySettings()) + + // When + let expectation = self.expectation(description: "Wait for failure") + validator.guessXMLRPCURLForSite("https://example.com", userAgent: "", success: { + XCTFail("Unexpected result: \($0)") + expectation.fulfill() + }, failure: { _ in + expectation.fulfill() + }) + + wait(for: [expectation], timeout: 2.0) + + // Then + XCTAssertEqual(schemes, ["https"]) + } + + func testItRejectsInsecureEndpointDiscoveredViaRSDForHTTPSInput() throws { + let responseInvalidPath = try XCTUnwrap(xmlrpcResponseInvalidPath) + stub(condition: isScheme("https") && isHost("www.apple.com") && isPath("/blog/xmlrpc.php")) { _ in + fixture(filePath: responseInvalidPath, status: 403, headers: nil) + } + + stub(condition: isScheme("https") && isHost("www.apple.com") && isPath("/blog")) { _ in + let html = """ + + + + + test site + + hello world + + """ + return HTTPStubsResponse(data: html.data(using: .utf8)!, statusCode: 200, headers: nil) + } + + // The plaintext endpoint answers with a valid methods list, but it must not be returned. + let responseListPath = try XCTUnwrap( + OHPathForFileInBundle("xmlrpc-response-list-methods.xml", Bundle.coreAPITestsBundle) + ) + stub(condition: isScheme("http") && isHost("www.apple.com") && isPath("/xmlrpc.php")) { _ in + fixture( + filePath: responseListPath, + status: 200, + headers: [ + "Content-Type": "application/xml" + ] + ) + } + + let failure = self.expectation(description: "returns error") + let validator = WordPressOrgXMLRPCValidator(makeUnsecuredAppTransportSecuritySettings()) + validator.guessXMLRPCURLForSite("https://www.apple.com/blog", userAgent: "test/1.0", success: { + XCTFail("Unexpected result: \($0)") + }) { error in + XCTAssertEqual(error as? WordPressOrgXMLRPCValidatorError, .insecureEndpoint) + failure.fulfill() + } + wait(for: [failure], timeout: 0.3) + } + let xmlrpcResponseInvalidPath = OHPathForFileInBundle( "xmlrpc-response-invalid.html", Bundle.coreAPITestsBundle diff --git a/WordPress/Classes/Utility/ZendeskUtils.swift b/WordPress/Classes/Utility/ZendeskUtils.swift index b8b8a12b16ef..59ef80ffc631 100644 --- a/WordPress/Classes/Utility/ZendeskUtils.swift +++ b/WordPress/Classes/Utility/ZendeskUtils.swift @@ -626,7 +626,7 @@ private extension ZendeskUtils { // Get email address from remote profile guard let username = blog.username, let password = blog.password, - let xmlrpc = blog.xmlrpc, + let xmlrpc = blog.xmlrpcURL?.absoluteString, let service = UsersService(username: username, password: password, xmlrpc: xmlrpc) else { return } diff --git a/WordPress/Classes/ViewRelated/Blog/Site Settings/SiteSettingsViewController.m b/WordPress/Classes/ViewRelated/Blog/Site Settings/SiteSettingsViewController.m index 6ab90dce3dfb..39107e3001c3 100644 --- a/WordPress/Classes/ViewRelated/Blog/Site Settings/SiteSettingsViewController.m +++ b/WordPress/Classes/ViewRelated/Blog/Site Settings/SiteSettingsViewController.m @@ -1067,7 +1067,7 @@ - (void)validateLoginCredentials [SVProgressHUD setDefaultMaskType:SVProgressHUDMaskTypeBlack]; [SVProgressHUD showWithStatus:NSLocalizedString(@"Authenticating", @"")]; - NSURL *xmlRpcURL = [NSURL URLWithString:self.blog.xmlrpc]; + NSURL *xmlRpcURL = self.blog.xmlrpcURL; WordPressOrgXMLRPCApi *api = [[WordPressOrgXMLRPCApi alloc] initWithEndpoint:xmlRpcURL userAgent:[WPUserAgent wordPressUserAgent]]; __weak __typeof__(self) weakSelf = self; [api checkCredentials:self.username password:self.password success:^(id __unused responseObject, NSHTTPURLResponse *__unused httpResponse) {