Subversion HTTP servers up to 1.7.8 (inclusive) are vulnerable to a remotely triggerable excessive memory use, which could result in a Denial of Service. Summary: ======== Subversion's mod_dav_svn Apache HTTPD server module will use excessive amounts of memory when a large number of properties are set or deleted on a node. This can lead to a DoS. There are no known instances of this problem being observed in the wild. Known vulnerable: ================= Subversion HTTPD servers <= 1.6.20 Subversion HTTPD servers 1.7.0 through 1.7.8 (inclusive) Known fixed: ============ Subversion 1.6.21 Subversion 1.7.9 svnserve (any version) is not vulnerable Details: ======== Setting or deleting a large number of properties on a node (file or directory) will result in a large amount of memory use. Due to the memory pooling behavior of Apache httpd and Subversion the completion of the request will not result in the immediate release of memory used. Repeated commits with the same properties will result in each httpd process plateauing out at some amount of memory. This could result in a Denial of Service if the system is exhausted of all available memory. Severity: ========= CVSSv2 Base Score: 4.9 CVSSv2 Base Vector: AV:N/AC:H/Au:S/C:N/I:N/A:C We consider this to be a medium risk vulnerability. In order to take advantage of this attack the attacker would require write access to the repository. Most configurations require authentication to commit changes and so anonymous users would not be able to use this attack in these cases. The impact of using this memory varies wildly based on operating system and httpd configuration. Some operating systems may kill off processes or crash if too much memory is used. The Apache httpd configuration option of MaxRequestsPerChild may restart a process after a certain number of requests and limit the impact of accidental exercise of this issue. However, a determined attacker could repeat the commit of a large number of properties or increase the number of properties sufficiently to mitigate any countermeasures. Recommendations: ================ We recommend all users to upgrade to Subversion 1.7.9. Users of Subversion 1.6.x or 1.7.x who are unable to upgrade may apply the included patch. New Subversion packages can be found at: http://subversion.apache.org/packages.html There is no effective configuration that can mitigate the issue entirely however the use of ulimit (or the equivalent) to set memory limits for processes may help prevent the impact affecting other services running on the same machine. References: =========== CVE-2013-1845 (Subversion) Reported by: ============ Alexander Klink, n.runs Patches: ======== Patch against 1.6.20: [[[ Index: subversion/mod_dav_svn/dav_svn.h =================================================================== --- subversion/mod_dav_svn/dav_svn.h (revision 1461956) +++ subversion/mod_dav_svn/dav_svn.h (working copy) @@ -254,6 +254,9 @@ struct dav_resource_private { interface (ie: /path/to/item?p=PEGREV]? */ svn_boolean_t pegged; + /* Cache any revprop change error */ + svn_error_t *revprop_error; + /* Pool to allocate temporary data from */ apr_pool_t *pool; }; Index: subversion/mod_dav_svn/deadprops.c =================================================================== --- subversion/mod_dav_svn/deadprops.c (revision 1461956) +++ subversion/mod_dav_svn/deadprops.c (working copy) @@ -49,8 +49,7 @@ struct dav_db { struct dav_deadprop_rollback { - dav_prop_name name; - svn_string_t value; + int dummy; }; @@ -134,6 +133,7 @@ save_value(dav_db *db, const dav_prop_name *name, { const char *propname; svn_error_t *serr; + apr_pool_t *subpool; /* get the repos-local name */ get_repos_propname(db, name, &propname); @@ -151,10 +151,14 @@ save_value(dav_db *db, const dav_prop_name *name, } /* Working Baseline or Working (Version) Resource */ + + /* A subpool to cope with mod_dav making multiple calls, e.g. during + PROPPATCH with multiple values. */ + subpool = svn_pool_create(db->resource->pool); if (db->resource->baselined) if (db->resource->working) serr = svn_repos_fs_change_txn_prop(db->resource->info->root.txn, - propname, value, db->resource->pool); + propname, value, subpool); else { /* ### VIOLATING deltaV: you can't proppatch a baseline, it's @@ -168,19 +172,29 @@ save_value(dav_db *db, const dav_prop_name *name, propname, value, TRUE, TRUE, db->authz_read_func, db->authz_read_baton, - db->resource->pool); + subpool); + /* mod_dav doesn't handle the returned error very well, it + generates its own generic error that will be returned to + the client. Cache the detailed error here so that it can + be returned a second time when the rollback mechanism + triggers. */ + if (serr) + db->resource->info->revprop_error = svn_error_dup(serr); + /* Tell the logging subsystem about the revprop change. */ dav_svn__operational_log(db->resource->info, svn_log__change_rev_prop( db->resource->info->root.rev, propname, - db->resource->pool)); + subpool)); } else serr = svn_repos_fs_change_node_prop(db->resource->info->root.root, get_repos_path(db->resource->info), - propname, value, db->resource->pool); + propname, value, subpool); + svn_pool_destroy(subpool); + if (serr != NULL) return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR, NULL, @@ -395,6 +409,7 @@ db_remove(dav_db *db, const dav_prop_name *name) { svn_error_t *serr; const char *propname; + apr_pool_t *subpool; /* get the repos-local name */ get_repos_propname(db, name, &propname); @@ -403,6 +418,10 @@ db_remove(dav_db *db, const dav_prop_name *name) if (propname == NULL) return NULL; + /* A subpool to cope with mod_dav making multiple calls, e.g. during + PROPPATCH with multiple values. */ + subpool = svn_pool_create(db->resource->pool); + /* Working Baseline or Working (Version) Resource */ if (db->resource->baselined) if (db->resource->working) @@ -419,11 +438,12 @@ db_remove(dav_db *db, const dav_prop_name *name) propname, NULL, TRUE, TRUE, db->authz_read_func, db->authz_read_baton, - db->resource->pool); + subpool); else serr = svn_repos_fs_change_node_prop(db->resource->info->root.root, get_repos_path(db->resource->info), - propname, NULL, db->resource->pool); + propname, NULL, subpool); + svn_pool_destroy(subpool); if (serr != NULL) return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR, "could not remove a property", @@ -598,19 +618,14 @@ db_get_rollback(dav_db *db, const dav_prop_name *name, dav_deadprop_rollback **prollback) { - dav_error *err; - dav_deadprop_rollback *ddp; - svn_string_t *propval; + /* This gets called by mod_dav in preparation for a revprop change. + mod_dav_svn doesn't need to make any changes during rollback, but + we want the rollback mechanism to trigger. Making changes in + response to post-revprop-change hook errors would be positively + wrong. */ - if ((err = get_value(db, name, &propval)) != NULL) - return err; + *prollback = apr_palloc(db->p, sizeof(dav_deadprop_rollback)); - ddp = apr_palloc(db->p, sizeof(*ddp)); - ddp->name = *name; - ddp->value.data = propval ? propval->data : NULL; - ddp->value.len = propval ? propval->len : 0; - - *prollback = ddp; return NULL; } @@ -618,12 +633,20 @@ db_get_rollback(dav_db *db, static dav_error * db_apply_rollback(dav_db *db, dav_deadprop_rollback *rollback) { - if (rollback->value.data == NULL) - { - return db_remove(db, &rollback->name); - } + dav_error *derr; - return save_value(db, &rollback->name, &rollback->value); + if (! db->resource->info->revprop_error) + return NULL; + + /* Returning the original revprop change error here will cause this + detailed error to get returned to the client in preference to the + more generic error created by mod_dav. */ + derr = dav_svn__convert_err(db->resource->info->revprop_error, + HTTP_INTERNAL_SERVER_ERROR, NULL, + db->resource->pool); + db->resource->info->revprop_error = NULL; + + return derr; } ]]] Patch against 1.7.8: [[[ Index: subversion/mod_dav_svn/deadprops.c =================================================================== --- subversion/mod_dav_svn/deadprops.c (revision 1458455) +++ subversion/mod_dav_svn/deadprops.c (working copy) @@ -168,6 +168,7 @@ save_value(dav_db *db, const dav_prop_name *name, const char *propname; svn_error_t *serr; const dav_resource *resource = db->resource; + apr_pool_t *subpool; /* get the repos-local name */ get_repos_propname(db, name, &propname); @@ -202,6 +203,9 @@ save_value(dav_db *db, const dav_prop_name *name, */ + /* A subpool to cope with mod_dav making multiple calls, e.g. during + PROPPATCH with multiple values. */ + subpool = svn_pool_create(db->resource->pool); if (db->resource->baselined) { if (db->resource->working) @@ -208,7 +212,7 @@ save_value(dav_db *db, const dav_prop_name *name, { serr = svn_repos_fs_change_txn_prop(resource->info->root.txn, propname, value, - resource->pool); + subpool); } else { @@ -219,7 +223,7 @@ save_value(dav_db *db, const dav_prop_name *name, TRUE, TRUE, db->authz_read_func, db->authz_read_baton, - resource->pool); + subpool); /* Prepare any hook failure message to get sent over the wire */ if (serr) @@ -242,20 +246,21 @@ save_value(dav_db *db, const dav_prop_name *name, dav_svn__operational_log(resource->info, svn_log__change_rev_prop( resource->info->root.rev, - propname, resource->pool)); + propname, subpool)); } } else if (resource->info->restype == DAV_SVN_RESTYPE_TXN_COLLECTION) { serr = svn_repos_fs_change_txn_prop(resource->info->root.txn, - propname, value, resource->pool); + propname, value, subpool); } else { serr = svn_repos_fs_change_node_prop(resource->info->root.root, get_repos_path(resource->info), - propname, value, resource->pool); + propname, value, subpool); } + svn_pool_destroy(subpool); if (serr != NULL) return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR, @@ -540,6 +545,7 @@ db_remove(dav_db *db, const dav_prop_name *name) { svn_error_t *serr; const char *propname; + apr_pool_t *subpool; /* get the repos-local name */ get_repos_propname(db, name, &propname); @@ -548,11 +554,15 @@ db_remove(dav_db *db, const dav_prop_name *name) if (propname == NULL) return NULL; + /* A subpool to cope with mod_dav making multiple calls, e.g. during + PROPPATCH with multiple values. */ + subpool = svn_pool_create(db->resource->pool); + /* Working Baseline or Working (Version) Resource */ if (db->resource->baselined) if (db->resource->working) serr = svn_repos_fs_change_txn_prop(db->resource->info->root.txn, - propname, NULL, db->resource->pool); + propname, NULL, subpool); else /* ### VIOLATING deltaV: you can't proppatch a baseline, it's not a working resource! But this is how we currently @@ -564,11 +574,12 @@ db_remove(dav_db *db, const dav_prop_name *name) propname, NULL, NULL, TRUE, TRUE, db->authz_read_func, db->authz_read_baton, - db->resource->pool); + subpool); else serr = svn_repos_fs_change_node_prop(db->resource->info->root.root, get_repos_path(db->resource->info), - propname, NULL, db->resource->pool); + propname, NULL, subpool); + svn_pool_destroy(subpool); if (serr != NULL) return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR, "could not remove a property", ]]]