diff --git a/NEWS b/NEWS index 8cc9061..98d72a2 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,11 @@ +New in version 1.29.1 +===================== + +Security fixes +-------------- +* Modify chronyc protocol to prevent amplification attacks (CVE-2014-0021) + (incompatible with previous protocol version, chronyc supports both) + New in version 1.29 =================== diff --git a/candm.h b/candm.h index a044c80..7e94caa 100644 --- a/candm.h +++ b/candm.h @@ -360,13 +360,24 @@ typedef struct { Version 5 : auth data moved to the end of the packet to allow hashes with different sizes, extended sources, tracking and activity reports, dropped subnets accessed and client accesses + + Version 6 : added padding to requests to prevent amplification attack, + changed maximum number of samples in manual list to 16 */ -#define PROTO_VERSION_NUMBER 5 +#define PROTO_VERSION_NUMBER 6 -/* The oldest protocol version that is compatible enough with - the current version to report a version mismatch */ -#define PROTO_VERSION_MISMATCH_COMPAT 4 +/* The oldest protocol versions that are compatible enough with the current + version to report a version mismatch for the server and the client */ +#define PROTO_VERSION_MISMATCH_COMPAT_SERVER 5 +#define PROTO_VERSION_MISMATCH_COMPAT_CLIENT 4 + +/* The first protocol version using padding in requests */ +#define PROTO_VERSION_PADDING 6 + +/* The maximum length of padding in request packet, currently + defined by CLIENT_ACCESSES_BY_INDEX and MANUAL_LIST */ +#define MAX_PADDING_LENGTH 396 /* ================================================== */ @@ -424,8 +435,13 @@ typedef struct { REQ_ReselectDistance reselect_distance; } data; /* Command specific parameters */ - /* authentication of the packet, there is no hole after the actual data - from the data union, this field only sets the maximum auth size */ + /* The following fields only set the maximum size of the packet. + There are no holes between them and the actual data. */ + + /* Padding used to prevent traffic amplification */ + uint8_t padding[MAX_PADDING_LENGTH]; + + /* Authentication data */ uint8_t auth[MAX_HASH_LENGTH]; } CMD_Request; @@ -465,6 +481,7 @@ typedef struct { #define STT_BADSUBNET 7 #define STT_ACCESSALLOWED 8 #define STT_ACCESSDENIED 9 +/* Deprecated */ #define STT_NOHOSTACCESS 10 #define STT_SOURCEALREADYKNOWN 11 #define STT_TOOMANYSOURCES 12 @@ -585,9 +602,10 @@ typedef struct { uint32_t next_index; /* the index 1 beyond those processed on this call */ uint32_t n_clients; /* the number of valid entries in the following array */ RPY_ClientAccesses_Client clients[MAX_CLIENT_ACCESSES]; + int32_t EOR; } RPY_ClientAccessesByIndex; -#define MAX_MANUAL_LIST_SAMPLES 32 +#define MAX_MANUAL_LIST_SAMPLES 16 typedef struct { Timeval when; @@ -599,6 +617,7 @@ typedef struct { typedef struct { uint32_t n_samples; RPY_ManualListSample samples[MAX_MANUAL_LIST_SAMPLES]; + int32_t EOR; } RPY_ManualList; typedef struct { diff --git a/client.c b/client.c index 467bfdd..1aaf714 100644 --- a/client.c +++ b/client.c @@ -1239,6 +1239,7 @@ static unsigned long token = 0; static int max_retries = 2; static int initial_timeout = 1000; +static int proto_version = PROTO_VERSION_NUMBER; /* This is the core protocol module. Complete particular fields in the outgoing packet, send it, wait for a response, handle retries, @@ -1257,13 +1258,13 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok) int read_length; int expected_length; int command_length; + int padding_length; int auth_length; struct timeval tv; int timeout; int n_attempts; fd_set rdfd, wrfd, exfd; - request->version = PROTO_VERSION_NUMBER; request->pkt_type = PKT_TYPE_CMD_REQUEST; request->res1 = 0; request->res2 = 0; @@ -1278,6 +1279,14 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok) n_attempts = 0; do { + request->version = proto_version; + command_length = PKL_CommandLength(request); + padding_length = PKL_CommandPaddingLength(request); + assert(command_length > 0 && command_length > padding_length); + + /* Zero the padding to avoid sending uninitialized data. This needs to be + done before generating auth data as it includes the padding. */ + memset(((char *)request) + command_length - padding_length, 0, padding_length); /* Decide whether to authenticate */ if (password) { @@ -1291,9 +1300,6 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok) auth_length = 0; } - command_length = PKL_CommandLength(request); - assert(command_length > 0); - /* add empty MD5 auth so older servers will not drop the request due to bad length */ if (!auth_length) { @@ -1400,8 +1406,8 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok) continue; } - bad_header = ((reply->version != PROTO_VERSION_NUMBER && - !(reply->version >= PROTO_VERSION_MISMATCH_COMPAT && + bad_header = ((reply->version != proto_version && + !(reply->version >= PROTO_VERSION_MISMATCH_COMPAT_CLIENT && ntohs(reply->status) == STT_BADPKTVERSION)) || (reply->pkt_type != PKT_TYPE_CMD_REPLY) || (reply->res1 != 0) || @@ -1416,6 +1422,19 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok) continue; } +#if PROTO_VERSION_NUMBER == 6 + /* Protocol version 5 is similar to 6 except there is no padding. + If a version 5 reply with STT_BADPKTVERSION is received, + switch our version and try again. */ + if (proto_version == PROTO_VERSION_NUMBER && + reply->version == PROTO_VERSION_NUMBER - 1) { + proto_version = PROTO_VERSION_NUMBER - 1; + continue; + } +#else +#error unknown compatibility with PROTO_VERSION - 1 +#endif + /* Good packet received, print out results */ #if 0 printf("Reply cmd=%d reply=%d stat=%d seq=%d utok=%08lx tok=%d\n", diff --git a/cmdmon.c b/cmdmon.c index 9696ff5..4de2c1c 100644 --- a/cmdmon.c +++ b/cmdmon.c @@ -265,11 +265,25 @@ prepare_socket(int family) void CAM_Initialise(int family) { + int i; + assert(!initialised); initialised = 1; assert(sizeof (permissions) / sizeof (permissions[0]) == N_REQUEST_TYPES); + for (i = 0; i < N_REQUEST_TYPES; i++) { + CMD_Request r; + int command_length, padding_length; + + r.version = PROTO_VERSION_NUMBER; + r.command = htons(i); + command_length = PKL_CommandLength(&r); + padding_length = PKL_CommandPaddingLength(&r); + assert(padding_length <= MAX_PADDING_LENGTH && padding_length <= command_length); + assert(command_length == 0 || command_length >= offsetof(CMD_Reply, data)); + } + utoken = (unsigned long) time(NULL); issued_tokens = returned_tokens = issue_pointer = 0; @@ -1708,7 +1722,13 @@ read_from_cmd_socket(void *anything) assert(0); } - allowed = ADF_IsAllowed(access_auth_table, &remote_ip) || localhost; + if (!(localhost || ADF_IsAllowed(access_auth_table, &remote_ip))) { + /* The client is not allowed access, so don't waste any more time + on him. Note that localhost is always allowed access + regardless of the defined access rules - otherwise, we could + shut ourselves out completely! */ + return; + } /* Message size sanity check */ if (read_length >= offsetof(CMD_Request, data)) { @@ -1718,13 +1738,13 @@ read_from_cmd_socket(void *anything) } if (expected_length < offsetof(CMD_Request, data) || + read_length < offsetof(CMD_Reply, data) || rx_message.pkt_type != PKT_TYPE_CMD_REQUEST || rx_message.res1 != 0 || rx_message.res2 != 0) { /* We don't know how to process anything like this */ - if (allowed) - CLG_LogCommandAccess(&remote_ip, CLG_CMD_BAD_PKT, cooked_now.tv_sec); + CLG_LogCommandAccess(&remote_ip, CLG_CMD_BAD_PKT, cooked_now.tv_sec); return; } @@ -1752,15 +1772,12 @@ read_from_cmd_socket(void *anything) if (!LOG_RateLimited()) { LOG(LOGS_WARN, LOGF_CmdMon, "Read command packet with protocol version %d (expected %d) from %s:%hu", rx_message.version, PROTO_VERSION_NUMBER, UTI_IPToString(&remote_ip), remote_port); } - if (allowed) - CLG_LogCommandAccess(&remote_ip, CLG_CMD_BAD_PKT, cooked_now.tv_sec); - if (rx_message.version >= PROTO_VERSION_MISMATCH_COMPAT) { + CLG_LogCommandAccess(&remote_ip, CLG_CMD_BAD_PKT, cooked_now.tv_sec); + + if (rx_message.version >= PROTO_VERSION_MISMATCH_COMPAT_SERVER) { tx_message.status = htons(STT_BADPKTVERSION); - /* add empty MD5 auth so older clients will not drop - the reply due to bad length */ - memset(((char *)&tx_message) + PKL_ReplyLength(&tx_message), 0, 16); - transmit_reply(&tx_message, &where_from, 16); + transmit_reply(&tx_message, &where_from, 0); } return; } @@ -1769,8 +1786,8 @@ read_from_cmd_socket(void *anything) if (!LOG_RateLimited()) { LOG(LOGS_WARN, LOGF_CmdMon, "Read command packet with invalid command %d from %s:%hu", rx_command, UTI_IPToString(&remote_ip), remote_port); } - if (allowed) - CLG_LogCommandAccess(&remote_ip, CLG_CMD_BAD_PKT, cooked_now.tv_sec); + + CLG_LogCommandAccess(&remote_ip, CLG_CMD_BAD_PKT, cooked_now.tv_sec); tx_message.status = htons(STT_INVALID); transmit_reply(&tx_message, &where_from, 0); @@ -1781,32 +1798,14 @@ read_from_cmd_socket(void *anything) if (!LOG_RateLimited()) { LOG(LOGS_WARN, LOGF_CmdMon, "Read incorrectly sized command packet from %s:%hu", UTI_IPToString(&remote_ip), remote_port); } - if (allowed) - CLG_LogCommandAccess(&remote_ip, CLG_CMD_BAD_PKT, cooked_now.tv_sec); + + CLG_LogCommandAccess(&remote_ip, CLG_CMD_BAD_PKT, cooked_now.tv_sec); tx_message.status = htons(STT_BADPKTLENGTH); transmit_reply(&tx_message, &where_from, 0); return; } - if (!allowed) { - /* The client is not allowed access, so don't waste any more time - on him. Note that localhost is always allowed access - regardless of the defined access rules - otherwise, we could - shut ourselves out completely! */ - - if (!LOG_RateLimited()) { - LOG(LOGS_WARN, LOGF_CmdMon, "Command packet received from unauthorised host %s port %d", - UTI_IPToString(&remote_ip), - remote_port); - } - - tx_message.status = htons(STT_NOHOSTACCESS); - transmit_reply(&tx_message, &where_from, 0); - - return; - } - /* OK, we have a valid message. Now dispatch on message type and process it. */ /* Do authentication stuff and command tokens here. Well-behaved diff --git a/faq.txt b/faq.txt index d9ed6dd..7cc00dd 100644 --- a/faq.txt +++ b/faq.txt @@ -153,23 +153,10 @@ arise. You should always make X quite high (e.g. 10) in this directive. S: Issues with chronyc -Q: I keep getting the error '510 No command access from this host --- Reply not authenticated'. +Q: I keep getting the error '506 Cannot talk to daemon'. Make sure that the chrony.conf file (on the computer where chronyd is running) has a 'cmdallow' entry for the computer you are running chronyc on. This -shouldn't be necessary for localhost, but some people still seem to need an -entry like 'cmdallow 127.0.0.1'. (It would be good to understand why problem -only affects some people). - -Q: I cannot log in from chronyc to carry out privileged tasks. -This is the second most common problem. - -Perhaps your /etc/chrony.keys file is badly formatted. Make sure that the -final line has a line feed at the end, otherwise the key on that line will work -as though the last character is missing. (Note, this bug was fixed in version -1.16.) - -Q: When I enter a command and hit <Return>, chronyc hangs -This probably means that chronyc cannot communicate with chronyd. +isn't necessary for localhost. Perhaps chronyd is not running. Try using the ps command (e.g. on Linux, 'ps -auxw') to see if it's running. Or try 'netstat -a' and see if the ports diff --git a/pktlength.c b/pktlength.c index 761dacd..03a5a7e 100644 --- a/pktlength.c +++ b/pktlength.c @@ -35,8 +35,8 @@ /* ================================================== */ -int -PKL_CommandLength(CMD_Request *r) +static int +command_unpadded_length(CMD_Request *r) { int type; type = ntohs(r->command); @@ -157,6 +157,149 @@ PKL_CommandLength(CMD_Request *r) } +/* ================================================== */ + +int +PKL_CommandLength(CMD_Request *r) +{ + int command_length; + + command_length = command_unpadded_length(r); + if (!command_length) + return 0; + + return command_length + PKL_CommandPaddingLength(r); +} + +/* ================================================== */ + +#define PADDING_LENGTH_(request_length, reply_length) \ + ((request_length) < (reply_length) ? (reply_length) - (request_length) : 0) + +#define PADDING_LENGTH(request_data, reply_data) \ + PADDING_LENGTH_(offsetof(CMD_Request, request_data), offsetof(CMD_Reply, reply_data)) + +int +PKL_CommandPaddingLength(CMD_Request *r) +{ + int type; + + if (r->version < PROTO_VERSION_PADDING) + return 0; + + type = ntohs(r->command); + + if (type < 0 || type >= N_REQUEST_TYPES) + return 0; + + switch (type) { + case REQ_NULL: + return PADDING_LENGTH(data, data.null.EOR); + case REQ_ONLINE: + return PADDING_LENGTH(data.online.EOR, data.null.EOR); + case REQ_OFFLINE: + return PADDING_LENGTH(data.offline.EOR, data.null.EOR); + case REQ_BURST: + return PADDING_LENGTH(data.burst.EOR, data.null.EOR); + case REQ_MODIFY_MINPOLL: + return PADDING_LENGTH(data.modify_minpoll.EOR, data.null.EOR); + case REQ_MODIFY_MAXPOLL: + return PADDING_LENGTH(data.modify_maxpoll.EOR, data.null.EOR); + case REQ_DUMP: + return PADDING_LENGTH(data.dump.EOR, data.null.EOR); + case REQ_MODIFY_MAXDELAY: + return PADDING_LENGTH(data.modify_maxdelay.EOR, data.null.EOR); + case REQ_MODIFY_MAXDELAYRATIO: + return PADDING_LENGTH(data.modify_maxdelayratio.EOR, data.null.EOR); + case REQ_MODIFY_MAXDELAYDEVRATIO: + return PADDING_LENGTH(data.modify_maxdelaydevratio.EOR, data.null.EOR); + case REQ_MODIFY_MAXUPDATESKEW: + return PADDING_LENGTH(data.modify_maxupdateskew.EOR, data.null.EOR); + case REQ_LOGON: + return PADDING_LENGTH(data.logon.EOR, data.null.EOR); + case REQ_SETTIME: + return PADDING_LENGTH(data.settime.EOR, data.manual_timestamp.EOR); + case REQ_LOCAL: + return PADDING_LENGTH(data.local.EOR, data.null.EOR); + case REQ_MANUAL: + return PADDING_LENGTH(data.manual.EOR, data.null.EOR); + case REQ_N_SOURCES: + return PADDING_LENGTH(data.n_sources.EOR, data.n_sources.EOR); + case REQ_SOURCE_DATA: + return PADDING_LENGTH(data.source_data.EOR, data.source_data.EOR); + case REQ_REKEY: + return PADDING_LENGTH(data.rekey.EOR, data.null.EOR); + case REQ_ALLOW: + return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR); + case REQ_ALLOWALL: + return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR); + case REQ_DENY: + return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR); + case REQ_DENYALL: + return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR); + case REQ_CMDALLOW: + return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR); + case REQ_CMDALLOWALL: + return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR); + case REQ_CMDDENY: + return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR); + case REQ_CMDDENYALL: + return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR); + case REQ_ACCHECK: + return PADDING_LENGTH(data.ac_check.EOR, data.null.EOR); + case REQ_CMDACCHECK: + return PADDING_LENGTH(data.ac_check.EOR, data.null.EOR); + case REQ_ADD_SERVER: + return PADDING_LENGTH(data.ntp_source.EOR, data.null.EOR); + case REQ_ADD_PEER: + return PADDING_LENGTH(data.ntp_source.EOR, data.null.EOR); + case REQ_DEL_SOURCE: + return PADDING_LENGTH(data.del_source.EOR, data.null.EOR); + case REQ_WRITERTC: + return PADDING_LENGTH(data.writertc.EOR, data.null.EOR); + case REQ_DFREQ: + return PADDING_LENGTH(data.dfreq.EOR, data.null.EOR); + case REQ_DOFFSET: + return PADDING_LENGTH(data.doffset.EOR, data.null.EOR); + case REQ_TRACKING: + return PADDING_LENGTH(data.tracking.EOR, data.tracking.EOR); + case REQ_SOURCESTATS: + return PADDING_LENGTH(data.sourcestats.EOR, data.sourcestats.EOR); + case REQ_RTCREPORT: + return PADDING_LENGTH(data.rtcreport.EOR, data.rtc.EOR); + case REQ_TRIMRTC: + return PADDING_LENGTH(data.trimrtc.EOR, data.null.EOR); + case REQ_CYCLELOGS: + return PADDING_LENGTH(data.cyclelogs.EOR, data.null.EOR); + case REQ_SUBNETS_ACCESSED: + case REQ_CLIENT_ACCESSES: + /* No longer supported */ + return 0; + case REQ_CLIENT_ACCESSES_BY_INDEX: + return PADDING_LENGTH(data.client_accesses_by_index.EOR, data.client_accesses_by_index.EOR); + case REQ_MANUAL_LIST: + return PADDING_LENGTH(data.manual_list.EOR, data.manual_list.EOR); + case REQ_MANUAL_DELETE: + return PADDING_LENGTH(data.manual_delete.EOR, data.null.EOR); + case REQ_MAKESTEP: + return PADDING_LENGTH(data.make_step.EOR, data.null.EOR); + case REQ_ACTIVITY: + return PADDING_LENGTH(data.activity.EOR, data.activity.EOR); + case REQ_RESELECT: + return PADDING_LENGTH(data.reselect.EOR, data.null.EOR); + case REQ_RESELECTDISTANCE: + return PADDING_LENGTH(data.reselect_distance.EOR, data.null.EOR); + case REQ_MODIFY_MINSTRATUM: + return PADDING_LENGTH(data.modify_minstratum.EOR, data.null.EOR); + case REQ_MODIFY_POLLTARGET: + return PADDING_LENGTH(data.modify_polltarget.EOR, data.null.EOR); + default: + /* If we fall through the switch, it most likely means we've forgotten to implement a new case */ + assert(0); + return 0; + } +} + /* ================================================== */ int diff --git a/pktlength.h b/pktlength.h index 026468d..fad4c30 100644 --- a/pktlength.h +++ b/pktlength.h @@ -33,6 +33,8 @@ extern int PKL_CommandLength(CMD_Request *r); +extern int PKL_CommandPaddingLength(CMD_Request *r); + extern int PKL_ReplyLength(CMD_Reply *r); #endif /* GOT_PKTLENGTH_H */