Remotely triggerable DoS vulnerability in svnserve 'get-deleted-rev'. Summary: ======== Subversion's svnserve server process may exit when a well-formed read-only request produces a particular answer. This can lead to disruption for users of the server. Known vulnerable: ================= Subversion svnserve servers through 1.9.10 (inclusive). Subversion svnserve servers 1.10.0 through 1.10.4 (inclusive). Subversion svnserve servers 1.11.0 through 1.11.1 (inclusive). Subversion svnserve servers 1.12.0 through 1.12.0 (inclusive). mod_dav_svn (any version) is not affected. Known fixed: ============ Subversion svnserve servers 1.9.11 Subversion svnserve servers 1.10.5 Subversion svnserve servers 1.12.1 (Subversion 1.11.x is not a supported release line.) Details: ======== Subversion svn:// connections, including svn+ssh:// and svn+://, use a custom network protocol [1] with Lisp-like syntax. The code implementing the protocol has dedicated codepaths for serialization of revision numbers into protocol integers. A particular client query could cause the server to attempt to reply with a revision number whose value is the invalid revision number constant `SVN_INVALID_REVNUM`, thereby triggering an assertion failure in the the serialization layer. [1] https://svn.apache.org/repos/asf/subversion/tags/1.10.0/subversion/libsvn_ra_svn/protocol Severity: ========= CVSSv3 Base Score: 6.5 (Medium) CVSSv3 Base Vector: CVSS:3.0/AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:N/A:H Exploitation results in denial of service by crashing an svnserve process. The impact of this differs depending on how svnserve is launched, including the different run modes selected by options such as "svnserve -d", "svnserve -T -d", "svnserve -t", and "svnserve -i". Recommendations: ================ We recommend all users to upgrade to a known fixed release of the Subversion svnserve server. The same releases also include changes in the client side. These client-side changes are not needed to fix the server vulnerability. Upgrading the clients to one of these releases provides an ordinary bug fix that make the case in question work correctly when operating against an upgraded server. - With a new client against a new server, such queries are now handled correctly. - With an old client against a new server, the client will report a more informative error message, and the server will not crash. - With a new client against an old server, the behaviour is the same as with an old client against an old server. Users who are unable to upgrade may apply the included patches. References: =========== CVE-2018-11782 (Subversion) Reported by: ============ Ace Olszowka, Build Master, Computers Unlimited Patches: ======== Patch against Subversion 1.12.0, 1.10.4: [[[ Index: subversion/libsvn_ra_svn/client.c =================================================================== --- subversion/libsvn_ra_svn/client.c +++ subversion/libsvn_ra_svn/client.c @@ -3102,25 +3102,38 @@ ra_svn_get_deleted_rev(svn_ra_session_t svn_revnum_t *revision_deleted, apr_pool_t *pool) { svn_ra_svn__session_baton_t *sess_baton = session->priv; svn_ra_svn_conn_t *conn = sess_baton->conn; + svn_error_t *err; path = reparent_path(session, path, pool); /* Transmit the parameters. */ SVN_ERR(svn_ra_svn__write_cmd_get_deleted_rev(conn, pool, path, peg_revision, end_revision)); /* Servers before 1.6 don't support this command. Check for this here. */ SVN_ERR(handle_unsupported_cmd(handle_auth_request(sess_baton, pool), N_("'get-deleted-rev' not implemented"))); - return svn_error_trace(svn_ra_svn__read_cmd_response(conn, pool, "r", - revision_deleted)); + err = svn_error_trace(svn_ra_svn__read_cmd_response(conn, pool, "r", + revision_deleted)); + /* The protocol does not allow for a reply of SVN_INVALID_REVNUM directly. + Instead, a new enough server returns SVN_ERR_ENTRY_MISSING_REVISION to + indicate the answer to the query is SVN_INVALID_REVNUM. (An older server + closes the connection and returns SVN_ERR_RA_SVN_CONNECTION_CLOSED.) */ + if (err && err->apr_err == SVN_ERR_ENTRY_MISSING_REVISION) + { + *revision_deleted = SVN_INVALID_REVNUM; + svn_error_clear(err); + } + else + SVN_ERR(err); + return SVN_NO_ERROR; } static svn_error_t * ra_svn_register_editor_shim_callbacks(svn_ra_session_t *session, svn_delta_shim_callbacks_t *callbacks) { Index: subversion/svnserve/serve.c =================================================================== --- subversion/svnserve/serve.c +++ subversion/svnserve/serve.c @@ -3513,14 +3513,27 @@ get_deleted_rev(svn_ra_svn_conn_t *conn, SVN_ERR(svn_ra_svn__parse_tuple(params, "crr", &path, &peg_revision, &end_revision)); full_path = svn_fspath__join(b->repository->fs_path->data, svn_relpath_canonicalize(path, pool), pool); SVN_ERR(log_command(b, conn, pool, "get-deleted-rev")); SVN_ERR(trivial_auth_request(conn, pool, b)); - SVN_ERR(svn_repos_deleted_rev(b->repository->fs, full_path, peg_revision, - end_revision, &revision_deleted, pool)); + SVN_CMD_ERR(svn_repos_deleted_rev(b->repository->fs, full_path, peg_revision, + end_revision, &revision_deleted, pool)); + + /* The protocol does not allow for a reply of SVN_INVALID_REVNUM directly. + Instead, return SVN_ERR_ENTRY_MISSING_REVISION. A new enough client + knows that this means the answer to the query is SVN_INVALID_REVNUM. + (An older client reports this as an error.) */ + if (revision_deleted == SVN_INVALID_REVNUM) + SVN_CMD_ERR(svn_error_createf(SVN_ERR_ENTRY_MISSING_REVISION, NULL, + "svn protocol command 'get-deleted-rev': " + "path '%s' was not deleted in r%ld-%ld; " + "NOTE: newer clients handle this case " + "and do not report it as an error", + full_path, peg_revision, end_revision)); + SVN_ERR(svn_ra_svn__write_cmd_response(conn, pool, "r", revision_deleted)); return SVN_NO_ERROR; } static svn_error_t * get_inherited_props(svn_ra_svn_conn_t *conn, Index: subversion/tests/libsvn_ra/ra-test.c =================================================================== --- subversion/tests/libsvn_ra/ra-test.c +++ subversion/tests/libsvn_ra/ra-test.c @@ -91,12 +91,47 @@ commit_changes(svn_ra_session_t *session SVN_ERR(editor->close_directory(dir_baton, pool)); SVN_ERR(editor->close_directory(root_baton, pool)); SVN_ERR(editor->close_edit(edit_baton, pool)); return SVN_NO_ERROR; } +/* Commit two revisions: add 'B', then delete 'A' */ +static svn_error_t * +commit_two_changes(svn_ra_session_t *session, + apr_pool_t *pool) +{ + apr_hash_t *revprop_table = apr_hash_make(pool); + const svn_delta_editor_t *editor; + void *edit_baton; + void *root_baton, *dir_baton; + + /* mkdir B */ + SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton, + revprop_table, + NULL, NULL, NULL, TRUE, pool)); + SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM, + pool, &root_baton)); + SVN_ERR(editor->add_directory("B", root_baton, NULL, SVN_INVALID_REVNUM, + pool, &dir_baton)); + SVN_ERR(editor->close_directory(dir_baton, pool)); + SVN_ERR(editor->close_directory(root_baton, pool)); + SVN_ERR(editor->close_edit(edit_baton, pool)); + + /* delete A */ + SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton, + revprop_table, + NULL, NULL, NULL, TRUE, pool)); + SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM, + pool, &root_baton)); + SVN_ERR(editor->delete_entry("A", SVN_INVALID_REVNUM, root_baton, pool)); + SVN_ERR(editor->close_directory(root_baton, pool)); + SVN_ERR(editor->close_edit(edit_baton, pool)); + + return SVN_NO_ERROR; +} + static svn_error_t * commit_tree(svn_ra_session_t *session, apr_pool_t *pool) { apr_hash_t *revprop_table = apr_hash_make(pool); const svn_delta_editor_t *editor; @@ -1781,12 +1816,62 @@ commit_locked_file(const svn_test_opts_t SVN_TEST_ASSERT(propval); SVN_TEST_STRING_ASSERT(propval->data, "propval"); return SVN_NO_ERROR; } +/* Cases of 'get-deleted-rev' that should return SVN_INVALID_REVNUM. */ +static svn_error_t * +test_get_deleted_rev_no_delete(const svn_test_opts_t *opts, + apr_pool_t *pool) +{ + svn_ra_session_t *ra_session; + svn_revnum_t revision_deleted; + + SVN_ERR(make_and_open_repos(&ra_session, + "test-repo-get-deleted-rev-no-delete", opts, + pool)); + SVN_ERR(commit_changes(ra_session, pool)); + SVN_ERR(commit_two_changes(ra_session, pool)); + + /* expect 'no deletion' in the range up to r2, when it is deleted in r3 */ + /* This was failing over RA-SVN where the 'get-deleted-rev' wire command's + prototype cannot directly represent that result. A new enough client and + server collaborate on a work-around implemented using an error code. */ + SVN_ERR(svn_ra_get_deleted_rev(ra_session, "A", 1, 2, + &revision_deleted, pool)); + SVN_TEST_INT_ASSERT(revision_deleted, SVN_INVALID_REVNUM); + + /* this connection should still be open: a simple case should still work */ + SVN_ERR(svn_ra_get_deleted_rev(ra_session, "A", 1, 3, + &revision_deleted, pool)); + SVN_TEST_INT_ASSERT(revision_deleted, 3); + + return SVN_NO_ERROR; +} + +/* Cases of 'get-deleted-rev' that should return an error. */ +static svn_error_t * +test_get_deleted_rev_errors(const svn_test_opts_t *opts, + apr_pool_t *pool) +{ + svn_ra_session_t *ra_session; + svn_revnum_t revision_deleted; + + SVN_ERR(make_and_open_repos(&ra_session, + "test-repo-get-deleted-rev-errors", opts, pool)); + SVN_ERR(commit_changes(ra_session, pool)); + + /* expect an error when searching up to r3, when repository head is r1 */ + SVN_TEST_ASSERT_ERROR(svn_ra_get_deleted_rev(ra_session, "A", 1, 3, + &revision_deleted, pool), + SVN_ERR_FS_NO_SUCH_REVISION); + + return SVN_NO_ERROR; +} + /* The test table. */ static int max_threads = 4; static struct svn_test_descriptor_t test_funcs[] = @@ -1817,10 +1902,14 @@ static struct svn_test_descriptor_t test SVN_TEST_OPTS_PASS(tunnel_run_checkout, "verify checkout over a tunnel"), SVN_TEST_OPTS_PASS(commit_empty_last_change, "check how last change applies to empty commit"), SVN_TEST_OPTS_PASS(commit_locked_file, "check commit editor for a locked file"), + SVN_TEST_OPTS_PASS(test_get_deleted_rev_no_delete, + "test get-deleted-rev no delete"), + SVN_TEST_OPTS_PASS(test_get_deleted_rev_errors, + "test get-deleted-rev errors"), SVN_TEST_NULL }; SVN_TEST_MAIN ]]] Patch against Subversion 1.9.10 [[[ Index: subversion/libsvn_ra_svn/client.c =================================================================== --- subversion/libsvn_ra_svn/client.c +++ subversion/libsvn_ra_svn/client.c @@ -2837,6 +2837,7 @@ { svn_ra_svn__session_baton_t *sess_baton = session->priv; svn_ra_svn_conn_t *conn = sess_baton->conn; + svn_error_t *err; /* Transmit the parameters. */ SVN_ERR(svn_ra_svn__write_cmd_get_deleted_rev(conn, pool, path, @@ -2846,8 +2847,20 @@ SVN_ERR(handle_unsupported_cmd(handle_auth_request(sess_baton, pool), N_("'get-deleted-rev' not implemented"))); - return svn_error_trace(svn_ra_svn__read_cmd_response(conn, pool, "r", - revision_deleted)); + err = svn_error_trace(svn_ra_svn__read_cmd_response(conn, pool, "r", + revision_deleted)); + /* The protocol does not allow for a reply of SVN_INVALID_REVNUM directly. + Instead, a new enough server returns SVN_ERR_ENTRY_MISSING_REVISION to + indicate the answer to the query is SVN_INVALID_REVNUM. (An older server + closes the connection and returns SVN_ERR_RA_SVN_CONNECTION_CLOSED.) */ + if (err && err->apr_err == SVN_ERR_ENTRY_MISSING_REVISION) + { + *revision_deleted = SVN_INVALID_REVNUM; + svn_error_clear(err); + } + else + SVN_ERR(err); + return SVN_NO_ERROR; } static svn_error_t * Index: subversion/svnserve/serve.c =================================================================== --- subversion/svnserve/serve.c +++ subversion/svnserve/serve.c @@ -3296,8 +3296,21 @@ svn_relpath_canonicalize(path, pool), pool); SVN_ERR(log_command(b, conn, pool, "get-deleted-rev")); SVN_ERR(trivial_auth_request(conn, pool, b)); - SVN_ERR(svn_repos_deleted_rev(b->repository->fs, full_path, peg_revision, - end_revision, &revision_deleted, pool)); + SVN_CMD_ERR(svn_repos_deleted_rev(b->repository->fs, full_path, peg_revision, + end_revision, &revision_deleted, pool)); + + /* The protocol does not allow for a reply of SVN_INVALID_REVNUM directly. + Instead, return SVN_ERR_ENTRY_MISSING_REVISION. A new enough client + knows that this means the answer to the query is SVN_INVALID_REVNUM. + (An older client reports this as an error.) */ + if (revision_deleted == SVN_INVALID_REVNUM) + SVN_CMD_ERR(svn_error_createf(SVN_ERR_ENTRY_MISSING_REVISION, NULL, + "svn protocol command 'get-deleted-rev': " + "path '%s' was not deleted in r%ld-%ld; " + "NOTE: newer clients handle this case " + "and do not report it as an error", + full_path, peg_revision, end_revision)); + SVN_ERR(svn_ra_svn__write_cmd_response(conn, pool, "r", revision_deleted)); return SVN_NO_ERROR; } @@ -3768,7 +3781,7 @@ serve_params_t *params, apr_pool_t *scratch_pool) { - svn_error_t *err, *io_err; + svn_error_t *err; apr_uint64_t ver; const char *client_url, *ra_client_string, *client_string; apr_array_header_t *caplist; @@ -3900,11 +3913,12 @@ } if (err) { - log_error(err, b); - io_err = svn_ra_svn__write_cmd_failure(conn, scratch_pool, err); - svn_error_clear(err); - SVN_ERR(io_err); - return svn_ra_svn__flush(conn, scratch_pool); + /* Report these errors to the client before closing the connection. */ + err = svn_error_compose_create(err, + svn_ra_svn__write_cmd_failure(conn, scratch_pool, err)); + err = svn_error_compose_create(err, + svn_ra_svn__flush(conn, scratch_pool)); + return err; } SVN_ERR(svn_fs_get_uuid(b->repository->fs, &b->repository->uuid, Index: subversion/tests/libsvn_ra/ra-test.c =================================================================== --- subversion/tests/libsvn_ra/ra-test.c +++ subversion/tests/libsvn_ra/ra-test.c @@ -93,6 +93,41 @@ return SVN_NO_ERROR; } +/* Commit two revisions: add 'B', then delete 'A' */ +static svn_error_t * +commit_two_changes(svn_ra_session_t *session, + apr_pool_t *pool) +{ + apr_hash_t *revprop_table = apr_hash_make(pool); + const svn_delta_editor_t *editor; + void *edit_baton; + void *root_baton, *dir_baton; + + /* mkdir B */ + SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton, + revprop_table, + NULL, NULL, NULL, TRUE, pool)); + SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM, + pool, &root_baton)); + SVN_ERR(editor->add_directory("B", root_baton, NULL, SVN_INVALID_REVNUM, + pool, &dir_baton)); + SVN_ERR(editor->close_directory(dir_baton, pool)); + SVN_ERR(editor->close_directory(root_baton, pool)); + SVN_ERR(editor->close_edit(edit_baton, pool)); + + /* delete A */ + SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton, + revprop_table, + NULL, NULL, NULL, TRUE, pool)); + SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM, + pool, &root_baton)); + SVN_ERR(editor->delete_entry("A", SVN_INVALID_REVNUM, root_baton, pool)); + SVN_ERR(editor->close_directory(root_baton, pool)); + SVN_ERR(editor->close_edit(edit_baton, pool)); + + return SVN_NO_ERROR; +} + static svn_error_t * commit_tree(svn_ra_session_t *session, apr_pool_t *pool) @@ -842,6 +877,63 @@ } +/* Cases of 'get-deleted-rev' that should return SVN_INVALID_REVNUM. */ +static svn_error_t * +test_get_deleted_rev_no_delete(const svn_test_opts_t *opts, + apr_pool_t *pool) +{ + svn_ra_session_t *ra_session; + svn_revnum_t revision_deleted; + + SVN_ERR(make_and_open_repos(&ra_session, + "test-repo-get-deleted-rev-no-delete", opts, + pool)); + SVN_ERR(commit_changes(ra_session, pool)); + SVN_ERR(commit_two_changes(ra_session, pool)); + + /* expect 'no deletion' in the range up to r2, when it is deleted in r3 */ + /* This was failing over RA-SVN where the 'get-deleted-rev' wire command's + prototype cannot directly represent that result. A new enough client and + server collaborate on a work-around implemented using an error code. */ + SVN_ERR(svn_ra_get_deleted_rev(ra_session, "A", 1, 2, + &revision_deleted, pool)); + SVN_TEST_INT_ASSERT(revision_deleted, SVN_INVALID_REVNUM); + + /* this connection should still be open: a simple case should still work */ + SVN_ERR(svn_ra_get_deleted_rev(ra_session, "A", 1, 3, + &revision_deleted, pool)); + SVN_TEST_INT_ASSERT(revision_deleted, 3); + + return SVN_NO_ERROR; +} + +/* Cases of 'get-deleted-rev' that should return an error. */ +static svn_error_t * +test_get_deleted_rev_errors(const svn_test_opts_t *opts, + apr_pool_t *pool) +{ + svn_ra_session_t *ra_session; + svn_revnum_t revision_deleted; + svn_error_t *err; + + SVN_ERR(make_and_open_repos(&ra_session, + "test-repo-get-deleted-rev-errors", opts, pool)); + SVN_ERR(commit_changes(ra_session, pool)); + + /* expect an error when searching up to r3, when repository head is r1 */ + err = svn_ra_get_deleted_rev(ra_session, "A", 1, 3, &revision_deleted, pool); + + /* mod_dav_svn returns a generic error code for "500 Internal Server Error"; + * the other RA layers return the specific error code for "no such revision". + * We should make these consistent, but for now that's how it is. */ + if (opts->repos_url && strncmp(opts->repos_url, "http", 4) == 0) + SVN_TEST_ASSERT_ERROR(err, SVN_ERR_RA_DAV_REQUEST_FAILED); + else + SVN_TEST_ASSERT_ERROR(err, SVN_ERR_FS_NO_SUCH_REVISION); + + return SVN_NO_ERROR; +} + /* The test table. */ static int max_threads = 2; @@ -867,6 +959,10 @@ "check list has_props performance"), SVN_TEST_OPTS_PASS(tunnel_run_checkout, "verify checkout over a tunnel"), + SVN_TEST_OPTS_PASS(test_get_deleted_rev_no_delete, + "test get-deleted-rev no delete"), + SVN_TEST_OPTS_PASS(test_get_deleted_rev_errors, + "test get-deleted-rev errors"), SVN_TEST_NULL }; ]]]