This forum is provided to promote discussion amongst students enrolled in
CITS3007 Secure Coding.
If posting a question, it's suggested you check first whether your question
is answered in the unit Frequently Asked Questions (FAQ) list, and use the search box
(on the right) to see if an answer to your question has already been posted.
Please consider offering answers and suggestions to help other students!
And if you fix a problem by following a suggestion here,
it would be great if other interested students could see a short
"Great, fixed it!" followup message.
The revised project spec is available – there are a few minor improvements to wording, but other than that, no very significant changes beyond what's already been flagged in this forum.
You can see the exact changes made to the spec document itself here, and you can see the changes made to crypto.h by downloading it and comparing it to the original (using diff or an equivalent GUI tool such as meld).
Based on code I've seen in labs, here are a few development tips:
Remember to enable warnings when compiling your code, or you'll likely miss problems in your code that GCC is telling you about. A reasonable set of warnings is -Wall -Wextra -Wconversion. (You should also, ideally, be running static analysers over your code; but enabling warnings is the absolute bare minimum.)
Remember to add the -pedantic-errors flag when compiling - as discussed in last week's lecture, this disables GCC extensions to C. (There is a list of them here. Some I can't see much use for, such as zero-sized structs and arrays, others are quite handy, such as function attributes.)
If you don't use the Google sanitizers (especially UBSan and ASan) together with debugging symbols (-g), you're making life needlessly hard for yourself. Without them, debugging C can be difficult and stressful. With them, resolving bugs can often be as easy as it is in, say, Python.
Comments that do no more than say what anyone can already see from the code will result in a loss of marks (e.g. "// We call malloc to allocate memory", "// return 0 on success"). This is already pointed out in the standard rubric, but I mention it again in case you missed it.
Useful comments usually explain not what the code is doing, but why that approach has been taken (e.g. in order to validate untrusted input, or to comply with something in the written spec). Imagine that your code is being read by someone doing a security review of it[*] – how would you justify your approach to them?
A small number of students each year attempt to do something "clever" in their code. "Clever" code is nearly always less readable and harder to review for security flaws, and so results in a loss of marks.
There should be almost no reason to define your own macros – they, too, just make it harder for a reviewer to check that your code is correct and complies with the spec. In general, function-like macros are more error-prone and harder to review than normal functions, and are only appropriate for large projects (where their utility outweighs the loss in readability), which this is not.
Don't #define preprocessor symbols like TRUE or FALSE,† either. The spec doesn't refer to any such values (it only talks about functions returning 0 or 1), so your code shouldn't either. (Why would you make it harder for a reviewer to check that your code implements the spec?)
When you submit your final code, make sure it's in some sort of logical order. For instance, the functions which form the publicly-accessable API for your code (four cipher functions plus cli) should probably all be next to each other, so a reviewer can access them easily. If you've written helper functions, you could put them before or after the publicly exposed functions, whatever your preference is. (I usually see them put before, but practice varies.)
Hopefully those are useful – let me know if you have any queries about them.
Cheers,
Arran
[*] Because it is.
† I just give this as an example of code I've seen. In modern C versions, we have bool types and properly typed const variables, so there's even less reason for this sort of thing.