Address CodeQL gripe about uncontrolled format string in handling of GEARMAND_PORT

[esabol]

This merge request addresses CodeQL's gripe about an "uncontrolled format string" alert around line 626 of libgearman-server/gearmand.cc. The alert suggests that, if a user sets the GEARMAND_PORT environment variable to a string with percent characters in it, for example, it could result in a buffer overflow and/or crash the program. (Refer to meta-issue #406.) This merge request addresses that by limiting the string to at most 5 characters (since 65535 is the maximum port number) and then truncating the string at the first non-digit character.

I believe this PR also addresses issue #320 with Kubernetes setting GEARMAND_PORT to unexpected values by default.

[SpamapS]

Hi Ed. Thanks for taking this on. I don't love the approach of silently dropping non-numeric chars. The real problem is the way we're constructing the buffer for gearmand_gai_error. I'd much rather see that changed to be an argument with a %s as the format string.

[esabol]

@SpamapS wrote:

Hi Ed. Thanks for taking this on. I don't love the approach of silently dropping non-numeric chars. The real problem is the way we're constructing the buffer for gearmand_gai_error.

I suspect that getaddrinfo() is already silently dropping non-numeric characters in the port number, because that's the sort of thing that libc does all over the place, so this probably doesn't actually change much in that regard.

I'd much rather see that changed to be an argument with a %s as the format string.

It doesn't work that way. Or the gearmand_log_* functions are too labyrinthine for me to discern a way of doing that.

That said, I think changing the "%s:%s" on line 616 of libgearman-server/gearmand.cc to "%s:%d" might satisfy CodeQL's gripe. Would you be ok with that? That would only change the error message.

gearmand/libgearman-server/gearmand.cc

Line 616 in 728f3a5

int length= snprintf(buffer, sizeof(buffer), "%s:%s", gearmand->host ? gearmand->host : "<any>", port->port);

Or we could get rid of lines 614 to 620 entirely and change line 626 from

gearmand/libgearman-server/gearmand.cc

Line 626 in 728f3a5

return gearmand_gai_error(buffer, ret);

to simply gearmand_gai_error("getaddrinfo", ret);, which is exactly what the code does at

gearmand/libgearman-server/gearmand.cc

Line 642 in 728f3a5

gearmand_gai_error("getaddrinfo", ret);

[SpamapS]

Actually no, I forgot, ports can be non-numeric! /etc/services defines names for some ports. So we can't just ditch the non-numerics.

Honestly, the way the code uses message/format interchangeably is a bit disturbing. format should always be protected and ideally is only programmer-input never user-input.

I don't know how far down the rabbit hole you're up for going, but if we could make a gearmand_gai_error alternative that takes both a format and a message, and use that instead of building the buffer, that would be the ideal IMO.

Otherwise, yeah, maybe just drop the detail in the error message.

[esabol]

It's not gearmand_gai_error() that's the problem. The problem is gearmand_log_perror(), I think, and I'm not willing to go down that rabbit hole. (It's usage is far too widespread.) But that gives me an idea. We could just change how gearmand_log_gai_error() calls gearmand_log_perror().... Revision forthcoming.

[esabol]

@SpamapS : Updated. Please review the latest revision.

[esabol]

ARGH! I hate GitHub Actions so much! All CI workflows are failing. It all worked a month ago!