[GitHub] mike-jumper commented on a change in pull request #209: GUACAMOLE-637: Migrate strncpy(), strcat(), etc. to safer libguac implementations.
mike-jumper commented on a change in pull request #209: GUACAMOLE-637: Migrate
strncpy(), strcat(), etc. to safer libguac implementations.
URL: https://github.com/apache/guacamole-server/pull/209#discussion_r250464239
##
File path: src/protocols/rdp/rdp_fs.c
##
@@ -607,11 +608,10 @@ const char* guac_rdp_fs_read_dir(guac_rdp_fs* fs, int
file_id) {
int guac_rdp_fs_normalize_path(const char* path, char* abs_path) {
int i;
-int path_depth = 0;
+int path_depth = 1;
Review comment:
> What did you find?
The additional leading `""` path component is used to ensure the final,
normalized path has a leading backslash following `guac_strljoin()`. I'm adding
documentation to that effect.
> Are you planning on adding more unit tests in this PR?
Yes, to verify that these various normalization functions continue to
function as intended.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] jmuehlner commented on a change in pull request #209: GUACAMOLE-637: Migrate strncpy(), strcat(), etc. to safer libguac implementations.
jmuehlner commented on a change in pull request #209: GUACAMOLE-637: Migrate
strncpy(), strcat(), etc. to safer libguac implementations.
URL: https://github.com/apache/guacamole-server/pull/209#discussion_r250463281
##
File path: src/protocols/rdp/rdp_fs.c
##
@@ -607,11 +608,10 @@ const char* guac_rdp_fs_read_dir(guac_rdp_fs* fs, int
file_id) {
int guac_rdp_fs_normalize_path(const char* path, char* abs_path) {
int i;
-int path_depth = 0;
+int path_depth = 1;
Review comment:
What did you find? Are you planning on adding more unit tests in this PR?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mike-jumper commented on a change in pull request #209: GUACAMOLE-637: Migrate strncpy(), strcat(), etc. to safer libguac implementations.
mike-jumper commented on a change in pull request #209: GUACAMOLE-637: Migrate
strncpy(), strcat(), etc. to safer libguac implementations.
URL: https://github.com/apache/guacamole-server/pull/209#discussion_r250451962
##
File path: src/libguac/tests/string/strlcat.c
##
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include
+#include
+
+#include
+#include
+
+/**
+ * Verify guac_strlcat() behavior when string fits buffer without truncation.
+ * The return value of each call should be the length of the resulting string.
+ * Each resulting string should contain the full result of the contatenation,
+ * including null terminator.
+ */
+void test_string__strlcat() {
+
+char buffer[1024];
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "Apache ");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "Guacamole", sizeof(buffer)), 16);
+CU_ASSERT_STRING_EQUAL(buffer, "Apache Guacamole");
+CU_ASSERT_EQUAL(buffer[17], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "This is a test", sizeof(buffer)),
14);
+CU_ASSERT_STRING_EQUAL(buffer, "This is a test");
+CU_ASSERT_EQUAL(buffer[15], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "AB");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "X", sizeof(buffer)), 3);
+CU_ASSERT_STRING_EQUAL(buffer, "ABX");
+CU_ASSERT_EQUAL(buffer[4], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "X");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "", sizeof(buffer)), 1);
+CU_ASSERT_STRING_EQUAL(buffer, "X");
+CU_ASSERT_EQUAL(buffer[2], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "", sizeof(buffer)), 0);
+CU_ASSERT_STRING_EQUAL(buffer, "");
+CU_ASSERT_EQUAL(buffer[1], '\xFF');
+
+}
+
+/**
+ * Verify guac_strlcat() behavior when string must be truncated to fit buffer.
+ * The return value of each call should be the length that would result from
+ * concatenating the strings given an infinite buffer, however only as many
+ * characters as can fit should be appended to the string within the buffer,
+ * and the buffer should be null-terminated.
+ */
+void test_string__strlcat_truncate() {
+
+char buffer[1024];
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "Apache ");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "Guacamole", 9), 16);
+CU_ASSERT_STRING_EQUAL(buffer, "Apache G");
+CU_ASSERT_EQUAL(buffer[9], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "This is a test", 10), 14);
+CU_ASSERT_STRING_EQUAL(buffer, "This is a");
+CU_ASSERT_EQUAL(buffer[10], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "This ");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "is ANOTHER test", 6), 20);
+CU_ASSERT_STRING_EQUAL(buffer, "This ");
+CU_ASSERT_EQUAL(buffer[6], '\xFF');
+
+}
+
+/**
+ * Verify guac_strlcat() behavior with zero buffer sizes. The return value of
+ * each call should be the size of the input string, while the buffer remains
+ * untouched.
+ */
+void test_string__strlcat_nospace() {
+
+/* 0-byte buffer plus 1 guard byte (to test overrun) */
+char buffer[1] = { '\xFF' };
+
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "Guacamole", 0), 9);
+CU_ASSERT_EQUAL(buffer[0], '\xFF');
+
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "This is a test", 0), 14);
+CU_ASSERT_EQUAL(buffer[0], '\xFF');
+
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "X", 0), 1);
+CU_ASSERT_EQUAL(buffer[0], '\xFF');
+
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "", 0), 0);
+CU_ASSERT_EQUAL(buffer[0], '\xFF');
+
+}
+
+/**
+ * Verify guac_strlcat() behavior with unterminated buffers. With respect to
+ * the return value, the length of the string in the buffer should be
+ * considered equal to the size of the buffer, however the resulting buffer
+ * should not be null-terminated.
+ */
+void test_string__strlcat_nonull() {
Review comment:
This specific safeguard is now documented for `guac_strlcat()`.
[GitHub] mike-jumper commented on a change in pull request #209: GUACAMOLE-637: Migrate strncpy(), strcat(), etc. to safer libguac implementations.
mike-jumper commented on a change in pull request #209: GUACAMOLE-637: Migrate strncpy(), strcat(), etc. to safer libguac implementations. URL: https://github.com/apache/guacamole-server/pull/209#discussion_r250450650 ## File path: src/libguac/tests/string/strlcat.c ## @@ -0,0 +1,153 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include +#include + +#include +#include + +/** + * Verify guac_strlcat() behavior when string fits buffer without truncation. + * The return value of each call should be the length of the resulting string. + * Each resulting string should contain the full result of the contatenation, Review comment: Fixed to be less fun. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mike-jumper commented on a change in pull request #209: GUACAMOLE-637: Migrate strncpy(), strcat(), etc. to safer libguac implementations.
mike-jumper commented on a change in pull request #209: GUACAMOLE-637: Migrate
strncpy(), strcat(), etc. to safer libguac implementations.
URL: https://github.com/apache/guacamole-server/pull/209#discussion_r250449707
##
File path: src/libguac/tests/string/strlcat.c
##
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include
+#include
+
+#include
+#include
+
+/**
+ * Verify guac_strlcat() behavior when string fits buffer without truncation.
+ * The return value of each call should be the length of the resulting string.
+ * Each resulting string should contain the full result of the contatenation,
+ * including null terminator.
+ */
+void test_string__strlcat() {
+
+char buffer[1024];
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "Apache ");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "Guacamole", sizeof(buffer)), 16);
+CU_ASSERT_STRING_EQUAL(buffer, "Apache Guacamole");
+CU_ASSERT_EQUAL(buffer[17], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "This is a test", sizeof(buffer)),
14);
+CU_ASSERT_STRING_EQUAL(buffer, "This is a test");
+CU_ASSERT_EQUAL(buffer[15], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "AB");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "X", sizeof(buffer)), 3);
+CU_ASSERT_STRING_EQUAL(buffer, "ABX");
+CU_ASSERT_EQUAL(buffer[4], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "X");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "", sizeof(buffer)), 1);
+CU_ASSERT_STRING_EQUAL(buffer, "X");
+CU_ASSERT_EQUAL(buffer[2], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "", sizeof(buffer)), 0);
+CU_ASSERT_STRING_EQUAL(buffer, "");
+CU_ASSERT_EQUAL(buffer[1], '\xFF');
+
+}
+
+/**
+ * Verify guac_strlcat() behavior when string must be truncated to fit buffer.
+ * The return value of each call should be the length that would result from
+ * concatenating the strings given an infinite buffer, however only as many
+ * characters as can fit should be appended to the string within the buffer,
+ * and the buffer should be null-terminated.
+ */
+void test_string__strlcat_truncate() {
+
+char buffer[1024];
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "Apache ");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "Guacamole", 9), 16);
+CU_ASSERT_STRING_EQUAL(buffer, "Apache G");
+CU_ASSERT_EQUAL(buffer[9], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "This is a test", 10), 14);
+CU_ASSERT_STRING_EQUAL(buffer, "This is a");
+CU_ASSERT_EQUAL(buffer[10], '\xFF');
+
+memset(buffer, 0xFF, sizeof(buffer));
+strcpy(buffer, "This ");
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "is ANOTHER test", 6), 20);
+CU_ASSERT_STRING_EQUAL(buffer, "This ");
+CU_ASSERT_EQUAL(buffer[6], '\xFF');
+
+}
+
+/**
+ * Verify guac_strlcat() behavior with zero buffer sizes. The return value of
+ * each call should be the size of the input string, while the buffer remains
+ * untouched.
+ */
+void test_string__strlcat_nospace() {
+
+/* 0-byte buffer plus 1 guard byte (to test overrun) */
+char buffer[1] = { '\xFF' };
+
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "Guacamole", 0), 9);
+CU_ASSERT_EQUAL(buffer[0], '\xFF');
+
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "This is a test", 0), 14);
+CU_ASSERT_EQUAL(buffer[0], '\xFF');
+
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "X", 0), 1);
+CU_ASSERT_EQUAL(buffer[0], '\xFF');
+
+CU_ASSERT_EQUAL(guac_strlcat(buffer, "", 0), 0);
+CU_ASSERT_EQUAL(buffer[0], '\xFF');
+
+}
+
+/**
+ * Verify guac_strlcat() behavior with unterminated buffers. With respect to
+ * the return value, the length of the string in the buffer should be
+ * considered equal to the size of the buffer, however the resulting buffer
+ * should not be null-terminated.
+ */
+void test_string__strlcat_nonull() {
Review comment:
Confirmed that FreeBSD has the same behavior as our implementations. The
unit tests pass
[GitHub] skotfred closed pull request #70: Added HTML lang="en"
skotfred closed pull request #70: Added HTML lang="en" URL: https://github.com/apache/guacamole-website/pull/70 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] skotfred commented on issue #70: Added HTML lang="en"
skotfred commented on issue #70: Added HTML lang="en" URL: https://github.com/apache/guacamole-website/pull/70#issuecomment-457036533 I’ll clean it up and resubmit later. Thanks This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mike-jumper commented on issue #70: Added HTML lang="en"
mike-jumper commented on issue #70: Added HTML lang="en" URL: https://github.com/apache/guacamole-website/pull/70#issuecomment-457013508 Yeah, the idea behind the change is valid, but these changes need to be made through guacamole-manual, not to the auto-generated build output. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] skotfred opened a new pull request #105: Added HTML lang="en"
skotfred opened a new pull request #105: Added HTML lang="en" URL: https://github.com/apache/guacamole-manual/pull/105 Primarily an accessibility concern for screen reader software. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] necouchman commented on issue #70: Added HTML lang="en"
necouchman commented on issue #70: Added HTML lang="en" URL: https://github.com/apache/guacamole-website/pull/70#issuecomment-457010567 Okay, major issue I see so far is that you're proposing changes to a lot of files that are dynamically generated (pretty much anything under the `doc/` directory/path). I'm not opposed to adding it for the other files, but the dynamic ones are just going to get overridden by the process that dynamically generates them. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] skotfred opened a new pull request #70: Added HTML lang="en"
skotfred opened a new pull request #70: Added HTML lang="en" URL: https://github.com/apache/guacamole-website/pull/70 Primarily an accessibility concern for screen reader software. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] necouchman merged pull request #69: Document vulnerability CVE-2018-1340, fixed in 1.0.0.
necouchman merged pull request #69: Document vulnerability CVE-2018-1340, fixed in 1.0.0. URL: https://github.com/apache/guacamole-website/pull/69 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mike-jumper opened a new pull request #69: Document vulnerability CVE-2018-1340, fixed in 1.0.0.
mike-jumper opened a new pull request #69: Document vulnerability CVE-2018-1340, fixed in 1.0.0. URL: https://github.com/apache/guacamole-website/pull/69 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[SECURITY] CVE-2018-1340: Secure flag missing from Apache Guacamole session cookie
CVE-2018-1340: Secure flag missing from Apache Guacamole session cookie Versions affected: Apache Guacamole 0.9.4 through 0.9.14 Description: Prior to 1.0.0, Apache Guacamole used a cookie for client-side storage of the user's session token. This cookie lacked the "secure" flag, which could allow an attacker eavesdropping on the network to intercept the user's session token if unencrypted HTTP requests are made to the same domain. Mitigation: Users of Apache Guacamole 0.9.14 or older should upgrade to 1.0.0. Credit: We would like to thank Ross Golder for reporting this issue.
[GitHub] necouchman opened a new pull request #211: GUACAMOLE-693: Update copyright year to 2019.
necouchman opened a new pull request #211: GUACAMOLE-693: Update copyright year to 2019. URL: https://github.com/apache/guacamole-server/pull/211 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] necouchman opened a new pull request #367: GUACAMOLE-693: Update copyright year to 2019.
necouchman opened a new pull request #367: GUACAMOLE-693: Update copyright year to 2019. URL: https://github.com/apache/guacamole-client/pull/367 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] JoelB opened a new pull request #366: GUACAMOLE-713: Add support for changing Logback verbosity to Docker start script
JoelB opened a new pull request #366: GUACAMOLE-713: Add support for changing Logback verbosity to Docker start script URL: https://github.com/apache/guacamole-client/pull/366 JIRA issue: https://issues.apache.org/jira/browse/GUACAMOLE-713 This change would enable changing the Logback verbosity level using a LOGBACK_LEVEL environment variable passed to Docker using the start.sh script. Doing this via a guacamole.properties setting would be more elegant, but I'm not familiar enough with the codebase to implement that right now. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] daniquir commented on issue #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module
daniquir commented on issue #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module URL: https://github.com/apache/guacamole-client/pull/359#issuecomment-456836843 > Empty classes/interfaces are definitely an antipattern. If something about the design of the MongoDB extension is requiring this, that suggests something needs to be addressed in the overall design. It shouldn't be necessary, and the needs of the specific Morphia/MongoDB implementation shouldn't be the guiding factor of the generic extraction of the permission implementation. You are absolutely right, the use of interfaces marker is not the best solution. No problem, I will study the case to get the best solution to this problem. > The presence of models in the generic part of this is also a sign that things should be rethought. The models underlying these objects (or if there even are models) should be a concern of the implementation leveraging the generic permission / connection tracking library, not the library itself. There are no models in the generic part, but there are parts of the code in which the models are used, for that reason there are interfaces that refer to the model located in the specific part of JDBC or Morphia. If this is not expected, I will also revise and modify it if necessary. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
