Understood.
Here the corrected patch.
--
Mildis
Le 2015-11-01 18:43, Willy Tarreau a écrit :
Hi,
On Tue, Oct 27, 2015 at 05:56:41PM +0100, Mildis wrote:
Hi,
I???ve reworked the patch.
No warnings whatsoever, wether using GCC or LLVM.
checkpatch is OK, no warnings either.
Buffers are secured and all loops exit naturally or with an index
I???ve
checked against off-by-one.
OK, thanks.
I???ve made a custom loop to remove the brackets : I found it easier
than
to do arithmetic around numerous strlen calls.
Also, I???ve declared the address buffer of with a size of 48 as it
looks
that INET6_ADDRSTRLEN is sometime define???d as 48, sometimes as 46.
OK, comments below :
diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..04917e4 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -620,9 +620,53 @@ const char *invalid_domainchar(const char *name) {
struct sockaddr_storage *str2ip2(const char *str, struct
sockaddr_storage *sa, int resolve)
{
struct hostent *he;
+ /* max IPv6 length, including brackets and terminating NULL */
+ char tmpip[48];
+ /* point to str by default, if FQDN processing is required */
+ const char *tmpipptr;
+
+ /* check IPv6 with square brackets */
+ if (str[0] == '[') {
+ size_t iplength = strlen(str);
+
+ if (iplength < 4) {
+ /* minimal size is 4 when using brackets "[::]" */
+ goto fail;
+ }
+ else if (iplength >= sizeof(tmpip)) {
+ /* IPv6 literal can not be larger than tmpip */
+ goto fail;
+ }
+ else {
+ if (str[iplength - 1] != ']') {
+ /* if address started with bracket, it should
end with bracket */
+ goto fail;
+ }
+ else {
+ int srcidx, dstidx;
+
+ /* copy and strip the brackets */
+ for (srcidx = 1, dstidx = 0; dstidx < 47;
srcidx++, dstidx++) {
+ if (str[srcidx] == ']')
+ break;
+ tmpip[dstidx] = str[srcidx];
+ }
Here this loop is confusing, useless and apparently wrong since it
stops
on the first closing bracket while you tested for the presence of the
last one, so nothing guarantees it's the same. You know from the "if"
above that str[iplength - 1] == ']', so you know the length of the
address :
it's iplength - 2, so what you need instead of this loop is simply :
memcpy(tmpip, str + 1, iplength - 2);
tmpip[iplength - 2] = 0;
+
+ /* secure buffer */
+ for (; dstidx <= 47; dstidx++)
+ tmpip[dstidx] = '\0';
This is useless, you just needed tmpip[dstidx] = 0. Also please note
that every time a safety measure is overdesigned, it indicates a lack
of trust on the rest of the code. The comment "secure buffer" clearly
indicates this. There's nothing to secure here, just a string to
terminate.
+ /* switch to tmpip as we are working on a literal bracketed IPv6
*/
+ tmpipptr = tmpip;
You can avoid changing all the pointers below from str to tmpipptr by
simply doing "str = tmpip" here.
The changes to str2ip2() look OK though.
Regards,
Willy
From b68e5c7f2bb674e39ed94cdcbddc6daa8190a583 Mon Sep 17 00:00:00 2001
From: mildis <[email protected]>
Date: Mon, 26 Oct 2015 18:50:08 +0100
Subject: [PATCH] MINOR: config: allow IPv6 bracketed literals
---
src/standard.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 54 insertions(+), 8 deletions(-)
diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..969daf6 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -620,6 +620,33 @@ const char *invalid_domainchar(const char *name) {
struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, int resolve)
{
struct hostent *he;
+ /* max IPv6 length, including brackets and terminating NULL */
+ char tmpip[48];
+
+ /* check IPv6 with square brackets */
+ if (str[0] == '[') {
+ size_t iplength = strlen(str);
+
+ if (iplength < 4) {
+ /* minimal size is 4 when using brackets "[::]" */
+ goto fail;
+ }
+ else if (iplength >= sizeof(tmpip)) {
+ /* IPv6 literal can not be larger than tmpip */
+ goto fail;
+ }
+ else {
+ if (str[iplength - 1] != ']') {
+ /* if address started with bracket, it should end with bracket */
+ goto fail;
+ }
+ else {
+ memcpy(tmpip, str + 1, iplength - 2);
+ tmpip[iplength - 2] = '\0';
+ str = tmpip;
+ }
+ }
+ }
/* Any IPv6 address */
if (str[0] == ':' && str[1] == ':' && !str[2]) {
@@ -747,10 +774,12 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
* the first byte of the address.
* - "fd@" => an integer must follow, and is a file descriptor number.
*
- * Also note that in order to avoid any ambiguity with IPv6 addresses, the ':'
- * is mandatory after the IP address even when no port is specified. NULL is
- * returned if the address cannot be parsed. The <low> and <high> ports are
- * always initialized if non-null, even for non-IP families.
+ * IPv6 addresses can be declared with or without square brackets. When using
+ * square brackets for IPv6 addresses, the port separator (colon) is optional.
+ * If not using square brackets, and in order to avoid any ambiguity with
+ * IPv6 addresses, the last colon ':' is mandatory even when no port is specified.
+ * NULL is returned if the address cannot be parsed. The <low> and <high> ports
+ * are always initialized if non-null, even for non-IP families.
*
* If <pfx> is non-null, it is used as a string prefix before any path-based
* address (typically the path to a unix socket).
@@ -855,12 +884,29 @@ struct sockaddr_storage *str2sa_range(const char *str, int *low, int *high, char
}
else { /* IPv4 and IPv6 */
int use_fqdn = 0;
+ char *end = str2 + strlen(str2);
+ char *chr;
- port1 = strrchr(str2, ':');
- if (port1)
- *port1++ = '\0';
- else
+ /* search for : or ] whatever comes first */
+ for (chr = end-1; chr > str2; chr--) {
+ if (*chr == ']' || *chr == ':')
+ break;
+ }
+
+ if (*chr == ':') {
+ /* Found a colon before a closing-bracket, must be a port separator.
+ * This guarantee backward compatibility.
+ */
+ *chr++ = '\0';
+ port1 = chr;
+ }
+ else {
+ /* Either no colon and no closing-bracket
+ * or directly ending with a closing-bracket.
+ * However, no port.
+ */
port1 = "";
+ }
if (str2ip2(str2, &ss, 0) == NULL) {
use_fqdn = 1;
--
2.4.9 (Apple Git-60)
