Skip to content
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

Update imap-date.c to check if number of days is correct #198

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ftaurino
Copy link

@ftaurino ftaurino commented May 6, 2023

Add some basic checks to ensure tm_mday values is correct for the month entered, especially for doveadm search conditions

Add some basic checks to ensure tm_mday values is correct for the month entered, especially for doveadm search conditions
@sirainen
Copy link
Contributor

sirainen commented May 7, 2023

Looks like the following utc_mktime() already fully validates all the dates. However, we're mostly just ignoring the results and assuming the timestamp is too large or too small. I think something like the patch below would work better, except the 2038 check needs to be better somehow.

diff --git a/src/lib-imap/imap-date.c b/src/lib-imap/imap-date.c
index 2f04cabcce..885123ae4c 100644
--- a/src/lib-imap/imap-date.c
+++ b/src/lib-imap/imap-date.c
@@ -79,11 +79,11 @@ static const char *imap_parse_date_internal(const char *str, struct tm *tm)
 	return str;
 }
 
-static bool imap_mktime(struct tm *tm, time_t *time_r)
+static int imap_mktime(struct tm *tm, time_t *time_r)
 {
 	*time_r = utc_mktime(tm);
 	if (*time_r != (time_t)-1)
-		return TRUE;
+		return 1;
 
 	/* the date is outside valid range for time_t. it might still be
 	   technically valid though, so try to handle this case.
@@ -96,7 +96,8 @@ static bool imap_mktime(struct tm *tm, time_t *time_r)
 #else
 		*time_r = 0;
 #endif
-	} else {
+		return 0;
+	} else if (tm->tm_year >= 2038-1970) {
 		/* too high. return the highest allowed value.
 		   we shouldn't get here with 64bit time_t,
 		   but handle that anyway. */
@@ -105,8 +106,10 @@ static bool imap_mktime(struct tm *tm, time_t *time_r)
 #else
 		*time_r = (1UL << TIME_T_MAX_BITS) - 1;
 #endif
+		return 0;
+	} else {
+		return -1;
 	}
-	return FALSE;
 }
 
 bool imap_parse_date(const char *str, time_t *timestamp_r)
@@ -118,7 +121,8 @@ bool imap_parse_date(const char *str, time_t *timestamp_r)
 		return FALSE;
 
 	tm.tm_isdst = -1;
-	(void)imap_mktime(&tm, timestamp_r);
+	if (imap_mktime(&tm, timestamp_r) < 0)
+		return FALSE;
 	return TRUE;
 }
 
@@ -126,6 +130,7 @@ bool imap_parse_datetime(const char *str, time_t *timestamp_r,
 			 int *timezone_offset_r)
 {
 	struct tm tm;
+	int ret;
 
 	str = imap_parse_date_internal(str, &tm);
 	if (str == NULL)
@@ -157,9 +162,9 @@ bool imap_parse_datetime(const char *str, time_t *timestamp_r,
 	*timezone_offset_r = parse_timezone(str);
 
 	tm.tm_isdst = -1;
-	if (imap_mktime(&tm, timestamp_r))
+	if ((ret = imap_mktime(&tm, timestamp_r)) > 0)
 		*timestamp_r -= *timezone_offset_r * 60;
-	return TRUE;
+	return ret >= 0;
 }
 
 static void imap_to_date_tm(char buf[11], const struct tm *tm)

@sirainen
Copy link
Contributor

sirainen commented May 7, 2023

Maybe the check is needed only with 32bit time_t, and with 64bit time_t just handle all (time_t)-1 return values as invalid.

@ftaurino
Copy link
Author

ftaurino commented May 8, 2023

Without this check, you can search messages with "doveadm" with dates like "32-Jan-2022" or "31-Apr-2023", with very strange results. This simple check just prevent this situation, before calling imap_mktime.

@sirainen
Copy link
Contributor

sirainen commented May 8, 2023

But you get better correctness with the patch I added, e.g. you can't use "29-Feb-2023". Just would need to replace tm->tm_year >= 2038-1970 somehow better.

@ftaurino
Copy link
Author

ftaurino commented May 8, 2023

Perhaps we can consider this basic check a date input validation, working both on 32 and 64 bit systems and also after 2038.

@sirainen
Copy link
Contributor

It's unnecessary to add such basic validation though if it can be done perfectly instead.. I did a bit improved patch that seems to catch all the cases correctly with 32bit and 64bit time_t. Tracking internally in DOP-3196.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants