New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reading mosquitto.conf with line > 1023 chars does unexpected things (with patch) #652

Closed
edwin-oetelaar opened this Issue Dec 12, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@edwin-oetelaar

edwin-oetelaar commented Dec 12, 2017

while testing TLS I added a line :

ciphers ECDHE-RSA-AES256-GCM-SHA384:ECDHE-E.......

The line is 1600 chars long, the config reader fails unexpectedly :

Error: Unable to open configuration file.
Error: Unknown configuration variable ":DHE-RSA-CAMELLIA128-SHA:DHE-DSS-...

It also fails if the ciphers line starts with # and is commented out

Any line longer than 1023 will be read as a new configuration line starting at byte 1024... this has unexpected consequenses. (reading commented content as if it had not been commented out, possible security implications? )

The problem is in src/conf.c line 572

while(fgets(buf, 1024, fptr)){

the buffer is on the stack:

char buf[1024];

char buf[1024];

I could fix this and submit a patch if you like that. (please say so)

For my test setup I will just hack the buffer to 4096 bytes for now and continue testing.

@toast-uz

This comment has been minimized.

Contributor

toast-uz commented Dec 17, 2017

You are right. But try "openssl ciphers shortening strings" may minimize the length of the list of valid ciphers. See mosquitto.conf and https://www.openssl.org/docs/man1.0.2/apps/ciphers.html for details.

@edwin-oetelaar

This comment has been minimized.

edwin-oetelaar commented Dec 17, 2017

I think 3 things should be done.

  • 1: make the buffer 2048 bytes instead of 1024 bytes so long VALID lines will work without problem
  • 2: make reading more robust so very long COMMENT lines will not break the config reader
  • 3: stop reading config file and program termination on invalid lines in config file
@edwin-oetelaar

This comment has been minimized.

edwin-oetelaar commented Dec 17, 2017

Here is the fix (fully tested), but I can make a pull request if you want that.

Best regards,
Edwin van den Oetelaar

diff` --git a/src/conf.c b/src/conf.c
index a3e233d..452608b 100644
--- a/src/conf.c
+++ b/src/conf.c
@@ -540,7 +540,8 @@ int mqtt3_config_read(struct mqtt3_config *config, bool reload)
 int _config_read_file_core(struct mqtt3_config *config, bool reload, const char *file, struct config_recurse *cr, int level, int *lineno, FILE *fptr)
 {
        int rc;
-       char buf[1024];
+    static const uint32_t bufsize = 2048; /* line buffer size */
+       char buf[bufsize]; /* line buffer */
        char *token;
        int tmp_int;
        char *saveptr = NULL;
@@ -569,8 +570,12 @@ int _config_read_file_core(struct mqtt3_config *config, bool reload, const char
 
        *lineno = 0;
 
-       while(fgets(buf, 1024, fptr)){
+       while(fgets(buf, bufsize, fptr)){
                (*lineno)++;
+        if(strlen(buf) == bufsize-1){
+            _mosquitto_log_printf(NULL, MOSQ_LOG_ERR, "Error: Line too long in configuration.");
+            return MOSQ_ERR_INVAL;
+        }
                if(buf[0] != '#' && buf[0] != 10 && buf[0] != 13){
                        while(buf[strlen(buf)-1] == 10 || buf[strlen(buf)-1] == 13){
                                buf[strlen(buf)-1] = 0;

@edwin-oetelaar edwin-oetelaar changed the title from reading mosquitto.conf with line > 1023 chars does unexpected things to reading mosquitto.conf with line > 1023 chars does unexpected things (with patch) Dec 18, 2017

@ralight

This comment has been minimized.

Contributor

ralight commented Dec 21, 2017

Thanks for the report and the patch, I've fixed it in a slightly different way that means it should never be a problem again.

@ralight ralight added this to the fixes-next milestone Dec 21, 2017

@ralight ralight closed this Dec 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment